Menu
▾
▴
#159 De-thread HtmlUnit
Ensure that all background JS tasks are executed in a single thread, a model followed by most common browsers.
The attached patch introduces an EventLoop per WebClient. The EventLoop simply runs the earliest job among all the JsJobManagers corresponding to all WebWiindow(s) of the WebClient. To turn off the effect of the patch, you can set
webClient.isSingleThreaded_ = false; in WebClient.java and everything should behave as before.
Each patch that solves some tests without breaking others is good. Many thanks.
A few questions:
- could you provide the patch in unified diff format against SVN head? It would be easier to apply.
- do you see some value in having isSingleThreaded_ configurable (in fact name is wrong, even in this case HtmlUnit would use more than one thread)?
The only interest I see is when we would simulate Opera. As it isn't planned in a near future, I think that it is better to keep things simple and to remove the old behaviour (I think that we came to this point in a dev list discussion for some time)
- what about making EventLoop responsible for the Thread?
- why the name "EventLoop" when it is currently only for JS execution? Do you prepare a patch using EventLoop for all user "actions"? ;-)
I think that there is a synchronisation problem. I'll try to add a new test illustrating the problem.
Will update the patch against svn head. I don't see any long-term advantage in having isSingleThreaded_ configurable. The short term advantage, if any, is that it provides an easier transition path. For example, if a new test fails in the 'single-threaded' mode but passes in the previous mode, we know where the bug is. So an option could be to clean up the isSingleThreaded_ configuration stuff after the new code is solid. I personally prefer removing the previous code, but I wanted to get feedback before I do that. Should I remove all the configuration and the JavaScriptJobManagerImpl and test classes?
I agree that the name "singlethreaded" is misleading. Will change it into "singleBackgroundThread" if we decide to keep the option, for now.
I wanted to make the EventLoop not responsible for thread, so that it allows configurations where the user/controller can decide when to pump the event loop.
It would be nice to have the "EventLoop" responsible for all user actions :). I am currently not preparing a patch for it, but it will certainly be closer to how most browsers operate. Suggestions for a better name?
Do let me know what to do whether I should keep the isSingleThreaded_ configurable, and I can prepare an updated patch against the HtmlUnit head accordingly.
The synchronisation problem that I imagined doesn't exist. Sorry.
Any reason why the verification for "right time for a job" is made in setCurrentlyRunningJob rather than in the EventLoop?
It seems "correct" to do the check just before executing the job. That said, the event loop tries to check the "right time for a job" in deciding whether to sleep or not.
Any preference as to if I should clean up the singleThreaded configuration stuff or just rename it and leave it in for the short-term?
The attached patch file is produced using 'svn diff' and is against svn r5521. Patch is much cleaner (and smaller) than before. I removed a lot of formatting diff that was unnecessary. Got rid of 2 constructors in WebClient. Plus, fixed 2 small bugs.
I wanted to complain about the lack of feedback from your side... and I now see that you're waiting for reactions from my side since a few day. Seems that I can't rely on the emails from SF... :-(
Please make single threaded the default without any option to keep the current behaviour. This will make code cleaner.
I think that I prefer if the EventLoop (or whatever the name is) is responsible for the thread. I guess that it could be useful in environments when no Thread can be started... like GAE.
Can you explicit the change in Dojo102Test?
I thought you were away for a few days or I would have emailed you directly :(. Let us try to get this patch in as soon as possible. I will strive to keep my turnaround time low.
Just to confirm the changes you want me to make:
(1) Remove the isSingleThreadedBackgroundJs variable. Remove the JavaScriptJobManagerImpl class, its associated test class, and all references to JavaScriptJobManagerImpl. Rename JavaScriptJobManagerSingleThreadedImpl to JavaScriptJobManagerImpl. Similarly for its test class.
(2) Move the eventLoopThread from WebClient into EventLoop and associated changes.
(3) Do you want me to add a comment in Dojo102Test?
// this test now passes in HtmlUnit, after the move to a single JS thread for all background JS tasks.
Do let me know and I can go ahead with these changes.
Yes to (1) and (2).
To (3): I've wrongly looked at the patch and thought that it was a regression. As it is an improvement, everything is ok.
And yes, I want to quickly apply the patch. We can rework it later once it is applied when needed.
Attaching the updated patch. Contains all the changes (1), (2) and a single-line comment for (3) except that I did not rename JavaScriptJobManagerSingleThreadedImpl to JavaScriptJobManagerImpl. Just to avoid conveying the impression that the previous file was modified.
Please feel free to make minor changes to the patch and commit it, if everything else seems fine.
Just discovered a bug in the current patch while trying it on GWT. With the patch, an async Xhr becomes synchronous with respect to other JS jobs. The reason is the xhr code that creates a new JavaScriptJob for an async xhr. The code previously translated to starting a new thread and thus the xhr was async with respect to other JS jobs. Since a new JS job no longer runs in a separate thread, this is no longer the case.
I don't think that the patch changes the situation: it is already wrong. I have added a new unit test as NotYetImplemented showing it: XMLHttpRequest2Test.xhrDownloadInBackground.
This is currently wrong because for an XHR, the download and the JS callback execution are "one block" that gets executed within the same synchronize on the HtmlPage. This should be modified by calling some download method with a callback. WebClient.download should be the basis for that (needs to be refactored as well).
When trying to execute XMLHttpRequest2Test.xhrDownloadInBackground with your patch applied, I get an exception. Can you look at it:
Caused by: java.lang.ClassCastException: com.gargoylesoftware.htmlunit.javascript.background.JavaScriptFunctionJob cannot be cast to java.lang.Comparable
at java.util.PriorityQueue.siftUpComparable(PriorityQueue.java:578)
...
at com.gargoylesoftware.htmlunit.javascript.background.JavaScriptJobManagerSingleThreadedImpl.addJob(JavaScriptJobManagerSingleThreadedImpl.java:107)
at com.gargoylesoftware.htmlunit.javascript.host.Window.jsxFunction_setTimeout(Window.java:404)
I agree that the xhr implementation is incorrect; with the patch, it just becomes more incorrect :).
I just updated to the latest svn trunk with my patch. However, I am not able to reproduce the exception. Everything passes for me. From the exception message, it seems that in the execution, JavaScriptFunctionJob could not be cast to Comparable, which is odd since its ancestor JavaScriptJob implements Comparable. Perhaps, the new and old classes are somehow getting mixed. Perhaps look at the execution classpath?
Can you provide a unit test that is now green and that becomes red with your patch?
Concerning Comparable: it seems that a part of your patch was rejected, exactly the part containing the Comparable stuff.
I don't have a unit test that is now green and becomes red with the patch. It is a GWT test case (with a server side component) that passed before but fails now. Essentially, the test issues a XHR for a URL, but the servlet is configured to delay the response. The XHR is then immediately cancelled and the test checks whether the XHR callback was called. Btw, do any HtmlUnit tests have a server component so that I can adapt the GWT test?
Regarding "a part of the patch was rejected": did 'svn merge' fail? That seems surprising. Do you want me to upload an updated patch?
All tests extending WebDriverTestCase have a server component. Have a look at XMLHttpRequest2Test.xhrDownloadInBackground, it contains an XHR and the server is configured to wait before to answer. What do you mean with "The XHR is then immediately cancelled"?
I don't know why some part of the patch were rejected. I've applied it from Eclipse as usual.
I've run all the tests with patch applied and everything is fine... except Mootools. Even if you have increased the wait delay, it doesn't reliably pass on my machine (seems that you have a better one ;-)). I have to increase the wait delay to have it reliably green but it seems to show some other problems. I have to investigate more here.
Here are the bad news: memory leak and performance degradation :-(
With your patch applied, I have to increase the delay for waitForBackgroundJavaScriptStartingBefore in MooTools (what should not be, but ok, this method doesn't work 100%). So I have now:
client_.waitForBackgroundJavaScriptStartingBefore(200000);
When I run MooToolsTest from Eclipse with this setting, the tests are reliably green but I see that:
- the tests are far more slow than without the patch (same wait setting)
- the performance degrades along the 5 tests (IE6-8, FF2-3) and the log output becomes significantly slower what is a sign for a memory leak
Here are some execution time that I get when executing MooToolsTest from within Eclipse:
- without the patch:
IE6: 28 s
others: between 16s and 20s
It makes sense that the first one is slower due to JVM warmup
IE6: 31s
IE7: 28s
IE8: 47s
FF2: 72s
FF3: 141s
It seems that there is some warmup effect that explains better time for second unit test but all next unit tests are dramatically slower.
Do you have similar execution time?
For MooTools, with the patch, I see IE6: 21.8s, IE7, IE8: less than 16.5s, FF2, FF3: less than 15.5s. Without the patch, the times were IE6: 17s, IE7, IE8: less than 13s, FF2, FF3: less than 12s. I have also not seen any memory leak.
Theoretically, it makes sense that tests will take a little bit more time because what used to be in parallel is now sequential. For example, if there are two xhrs that take 5s each to complete, the previous time was 5 time, where as the time with the patch will increase to 10s. Of course, this can be fixed with xhr handling.
So there is a bit of performance hit and presumably much of it can be fixed once the xhr handling improves and some more performance optimization is done (I have done almost nothing so far). If this is the only issue, I would vote for putting the patch in. What do you say?
Also, from the MooTools numbers, my numbers are only about 20% worse, but in your case, those seem to be much worse. Is it possible that our code bases have diverged?
Your times look quite good. I now understand the regression case you mentioned earlier: it is only when xhr is performed in an other page. I don't really think that such a case occurs in MooTools tests.
I have to understand why you don't have any real performance diminution in MooTools tests with the patch whereas my results are alarming. Perhaps has it to do with my problems applying the patch. I'll look at it on Monday again, it's now too late ;-)
Ok. Good night!
Do let me know if you have any problems applying the patch.
How did you generate your patch? Even if I revert my working copy to revision 1531, I can't successfully apply your patch.
What do you mean with svn merge? As far as I know, svn merge can only compare two trees. Here we have only one.
Apply using 'patch -p1' against r5531
I generated the patch using 'svn diff'. I actually work in git. So I generated the patch using git, patched it into a dir checked out into svn, and generated the patch I uploaded using 'svn diff'. It did not work because I myself was unable to patch the generated diff file.
Finally generated a diff file using the plain old diff command. I have uploaded the updated file -- it should apply cleanly. Found out that 'svn diff' was not correctly handling $Revision$ keyword. What is the proper way to generate the diff file, especially for a deleted file?
Also, 'svn merge' was a typo. I simply meant 'patch'.
The format of this patch is far better! It was ok for deleted files as well.
The problems persist :-(
MooTools121Test doesn't reliably pass on my computer with your changes. A far longer delay in waitForBackgroundJavaScriptStartingBefore allows to have green tests but not in an acceptable delay (99 seconds for FF3). I've tried to use a simple Thread.sleep instead to see if the problem is not in the waitForBackgroundJavaScriptStartingBefore method but the most of the tests are even with 40s sleep!
I've asked Ahmed to apply the patch locally to compare the execution time.
Assuming that I would get the same numbers as you, meaning about 20% worse than current numbers. 20% is quite a lot and I think that we should try to reduce it. As you said, one point may be the fact that XHR requests (in different pages) don't run in parallel anymore. The problem already exists partially in the case of XHR in the same page. Fixing it right now would allow to avoid this cause of performance regression and would allow to better compare the impact of your patch.
I see an other location in your patch where some time may be lost: the Thread.sleep(10). What about using wait/notify instead?