Closed
Bug 517329
Opened 15 years ago
Closed 15 years ago
deep aborts are not properly blacklisted
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
DUPLICATE
of bug 517973
People
(Reporter: luke, Assigned: luke)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
The following loop will try to record, and abort, N times:
for (var i=0; i<100; ++i)
[1,2,3].map(function() { return 9 })
Comment 1•15 years ago
|
||
The patch for bug 471214 suggests joining optimized closures passed as funargs as well as assigned to properties. We would need a read barrier on formal parameters to catch escape attempts in the callee and auto-clone, and of course we'd allow invocation of the funarg without cloning.
/be
Assignee | ||
Comment 2•15 years ago
|
||
Yeah, that original summary was a bit incomplete: this loop should be blacklisted after a fixed number of attempts to record. E.g., if you do:
for (var i=0; i<100; ++i)
"abcd".replace("b", function(){return "B"});
it stops after the third try in my testing.
Comment 3•15 years ago
|
||
Luke pointed out my burblings in comment 1 don't matter as far as tracing goes, if map is self-hosted, since the JIT guards on JSFunction*, which does not vary even if we clone 9 times. But comment 1's idea may be worth it to avoid making 9 clones of the same silly compiler-created exemplar for the lambda.
/be
Updated•15 years ago
|
Summary: loop over Array.map not being blacklisted → deep aborts are not properly blacklisted
Comment 4•15 years ago
|
||
This can create rather pathological performance problems.
Flags: blocking1.9.2?
Assignee | ||
Comment 5•15 years ago
|
||
I found the root cause, but I don't quite know enough about the situation to know whether my "fix" is really a fix. Perhaps someone more knowing can answer my question below?
So, blacklisting for this situation is supposed to happen in js_AbortRecording, called from monitorRecording if tr->wasDeepAborted. To blacklist, js_AbortRecording needs the recorder's fragment, however, for the above example, this is getting set to NULL by js_Interpret calling tr->removeFragmentReferences(). The removeFragmentReferences call only happens if the trace recorder has already been deep aborted but, since js_Interpret deep aborts right before exit, this is indeed the case when js_Invoke is called multiple times (once for each array element in the 'map' example).
So, a naive fix is simply:
jsinterpret.cpp:2814
- if (tr) {
- if (!tr->wasDeepAborted())
- tr->removeFragmentReferences();
- else
- tr->pushAbortStack();
+ if (tr && !tr->wasDeepAborted())
+ tr->pushAbortStack()
But perhaps removeFragmentReferences was necessary for some other case? Anyone know?
Incidentally, wrt comment 2, String.replace isn't immune, a "global" replace will call js_Invoke multiple times and have the same problem.
Comment 6•15 years ago
|
||
Ask dvander for review. He wrote the abort stack code.
Assignee | ||
Comment 7•15 years ago
|
||
The solution is basically what was discussed in the comment. The logic was a bit hairy before, so this patch cleans it up a bit and lays down some commentage.
Also, TraceRecorder::entryTypeMap is removed because its dead.
Assignee | ||
Comment 8•15 years ago
|
||
Diff-ing js/tests w/ and w/o the patch, two bugs get fixed! From a quick gander at their summaries, I think they are fixed by the patch, which would be cool.
> js1_8_1/trace/regress-452498-01.js
> js1_8_1/trace/regress-451974-02.js
Assignee | ||
Comment 9•15 years ago
|
||
Comment 8 naturally leads to the question: what about the benchmarks?
With --runs=30, sunspider is 2.1% faster and significant.
With --runs=3, v8 is 10% faster:
v8: 1.109x as fast 16032.0ms +/- 6.2% 14458.7ms +/- 6.7% significant
crypto: 1.181x as fast 951.0ms +/- 0.5% 805.3ms +/- 4.1% significant
deltablue: 1.181x as fast 8565.7ms +/- 8.8% 7255.3ms +/- 14.2% significant
Perhaps we need a special test in trace-test that runs all our benchmarks twice, and asserts that the second time we don't record.
Comment 10•15 years ago
|
||
(In reply to comment #9)
> Comment 8 naturally leads to the question: what about the benchmarks?
> With --runs=30, sunspider is 2.1% faster and significant.
> With --runs=3, v8 is 10% faster
luke++! Nice catch!
Assignee | ||
Comment 11•15 years ago
|
||
p.s., bug 517973 fixes this bug as part of removing deep aborts.
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Updated•15 years ago
|
Priority: -- → P2
Assignee | ||
Comment 12•15 years ago
|
||
Weeeell, in response to bug 517973 comment 5, I did more measuring only to find that the perf win seems to have gone away, so I looked more at sunspider and realized, to my great dismay, that the standalone-shell version of sunspider (sunspider --shell=.../js --args=-j) runs each iteration in a separate process, hence it is doing recording on every iteration!! Furthermore, I find that it records a wildly different number of traces each time. (dvander explains this is probably the oracle.) So, since I was measuring an improvement that reduces the number of aborts in the standalone sunspider test, it seems my performance finding is a fluke.
As I understand it, our official numbers come from the browser, which, I assume, will record only on the first (ignored) iteration. Perhaps, then, we should host our own branch of the sunspider standalone test that runs all the tests in the same context so that we could get more realistic development feedback?
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #12)
Again, my benchmark assumptions are wrong; the browser version of sunspider also records every iteration. I'm still not sure about the transient 10% speedup.
Assignee | ||
Updated•15 years ago
|
Attachment #401574 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•