deep aborts are not properly blacklisted

RESOLVED DUPLICATE of bug 517973

Status

()

P2
normal
RESOLVED DUPLICATE of bug 517973
9 years ago
9 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
x86
Linux
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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 })
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

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

9 years ago
Summary: loop over Array.map not being blacklisted → deep aborts are not properly blacklisted

Comment 4

9 years ago
This can create rather pathological performance problems.
Flags: blocking1.9.2?
(Assignee)

Comment 5

9 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

9 years ago
Ask dvander for review. He wrote the abort stack code.
(Assignee)

Comment 7

9 years ago
Created attachment 401574 [details] [diff] [review]
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.
Assignee: general → lw
Status: NEW → ASSIGNED
Attachment #401574 - Flags: review?
(Assignee)

Comment 8

9 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

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

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

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2+

Updated

9 years ago
Priority: -- → P2
(Assignee)

Comment 12

9 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

9 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)

Comment 14

9 years ago
Fixed in bug 517973
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

9 years ago
Attachment #401574 - Flags: review?
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 517973
You need to log in before you can comment on or make changes to this bug.