search

puniverse / quasar

Fibers, Channels and Actors for the JVM
http://docs.paralleluniverse.co/quasar/
Other
4.56k stars 574 forks source link

Allow Quasar to work with multiple classloaders. #209

Closed mikehearn closed 8 years ago

mikehearn commented 8 years ago

Hi Ron, Fabio,

This commit is the work of my colleague Matthew Nesbit. It upgrades Quasar so it can operate with multiple classloaders, which is useful if you wish to dynamically load fiber implementations from plugins, inside a sandbox etc. The unit tests all pass for me, and we have it working internally, but there may still be issues or a need to tweak things further.

circlespainter commented 8 years ago

Thanks a lot Matthew and Mike! We'll look into the PR soon, which could also close #196 [edit] when merged, right? If you have the time, some basic unit test(s) with multiple classloaders would really be great.

mikehearn commented 8 years ago

@circlespainter I've pushed a new version from Matthew that has unit tests as well. How does it look?

circlespainter commented 8 years ago

It looks good but the new tests seem to pass even on master (i.e. without the classloaders patch), shouldn't they pass only with the improvements instead?

mikehearn commented 8 years ago

OK, so here's what I understand having chatted to Matthew about this some more: the current code in git master actually does work with multiple classloaders, but only kind of by accident, and the errors you get back when something goes wrong are incomprehensible as a result.

These patches add explicit tests and explicit tracking of classloaders, meaning that things work by design rather than by accident, and the results when something goes wrong are now better. At least, that's my high level understanding.

circlespainter commented 8 years ago

I studied the PR and I think it contains important improvements but I also think that tests should identify them as precisely as possible and one important reason is that this allows to detect immediately when changes cause regressions in them.

If you know for sure that the previous code got into trouble and that it was hard to figure out what was happening, I think you've also seen that trouble already: can you recall/reconstruct it and tailor the tests to it, or just tell how to reproduce it? The only one I could identify so far (and write a test tailored to it) was extra unneeded instrumentation but that one isn't really supposed to cause any trouble, rather just a potential (and tiny, unless the triggering circumstance occurs everywhere all the time, which is unlikely) slowdown.

pron commented 8 years ago

I have merged this PR but made some refactoring to QuasarInstrumentor and MethodDatabase as a result. The instrumentor does not have a default DB now.

mikehearn commented 8 years ago

Thanks!