517329 - deep aborts are not properly blacklisted

Status: RESOLVED DUPLICATE of bug 517973

(Core :: JavaScript Engine, defect, P2)

Description

[Luke Wagner [:luke]]

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

[Brendan Eich [:brendan]]

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

Comment 2

[Luke Wagner [:luke]]

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

[Brendan Eich [:brendan]]

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

Comment 4

[Andreas Gal :gal]

This can create rather pathological performance problems.

Comment 5

[Luke Wagner [:luke]]

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

[Andreas Gal :gal]

Ask dvander for review. He wrote the abort stack code.

Comment 7

[Luke Wagner [:luke]]

Attachment: fix

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.

Comment 8

[Luke Wagner [:luke]]

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

Comment 9

[Luke Wagner [:luke]]

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

[David Mandelin [:dmandelin]]

(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!

Comment 11

[Luke Wagner [:luke]]

p.s., bug 517973 fixes this bug as part of removing deep aborts.

Comment 12

[Luke Wagner [:luke]]

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?

Comment 13

[Luke Wagner [:luke]]

(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.

Comment 14

[Luke Wagner [:luke]]

Fixed in bug 517973