Closed Bug 894948 Opened 7 years ago Closed 7 years ago

Remove effectively-dead analysis-related code from interpreter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: till, Assigned: till)

Details

Attachments

(2 files)

There's some code in the interpreter that does some analysis-related work and then, at the end, checks if compartment()->activeAnalysis is true. Which it shouldn't ever be if we're running in the interpreter. The attached patch removes that code.
I tested SunSpider 1.0 and v8-v6: this has no influence on perf whatsoever.
Attachment #777157 - Flags: review?(jdemooij)
Comment on attachment 777157 [details] [diff] [review]
Remove effectively-dead analysis-related code from interpreter

Review of attachment 777157 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch!

::: js/src/vm/Interpreter-inl.h
@@ -364,5 @@
> -                jsbytecode *pc = NULL;
> -                types::TypeScript::GetPcScript(cx, &script, &pc);
> -
> -                if (script->hasAnalysis())
> -                    script->analysis()->getCode(pc).nonNativeGetElement = true;

We can remove ScriptAnalysis::nonNativeGetElement and ScriptAnalysis::getStringElement now. IonBuilder reads nonNativeGetElement but it will never be set.

::: js/src/vm/Interpreter.cpp
@@ +1248,1 @@
>      if (obj->isNative() && JSID_IS_INT(id)) {

I think you can remove this whole |if|, the maybeScript/pc arguments, the SetObjectElement overload that takes script/pc and ScriptAnalysis::arrayWriteHole. IonBuilder gets this info from the baseline IC's now.
Attachment #777157 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/95a4b87a0583

5 files changed, 4 insertions(+), 81 deletions(-)
Status: NEW → ASSIGNED
Unfortunately, I was able to bisect the big Octane-Crypto regression to this patch.  On the bright side, if this is supposed to be dead code, you should be able to bisect hunk by hunk to find the offender ;)
(In reply to Luke Wagner [:luke] from comment #4)
> Unfortunately, I was able to bisect the big Octane-Crypto regression to this
> patch.  On the bright side, if this is supposed to be dead code, you should
> be able to bisect hunk by hunk to find the offender ;)

Ah, thanks for bisecting this. I think I already know which hunks are the problem here: I, stupidly, didn't benchmark again after updating the patch with the review comments. Rumors of ScriptAnalysis::arrayWriteHole's death have apparently been exaggerated.
The regression was indeed caused by removing ScriptAnalysis::arrayWriteHole. I wouldn't have caught it even had I run the benchmarks again: I tested with the SunSpider-version of v8-v6, the crypto benchmark of which doesn't regress. Yay for benchmarking!
Attachment #777476 - Flags: review?(jdemooij)
(In reply to Till Schneidereit [:till] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/95a4b87a0583
> 
> 5 files changed, 4 insertions(+), 81 deletions(-)

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/353d7b3d98ef for the regression.
Comment on attachment 777476 [details] [diff] [review]
Backout of the removal of ScriptAnalysis::arrayWriteHole

Review of attachment 777476 [details] [diff] [review]:
-----------------------------------------------------------------

Hrm sorry for not catching this.

What's going on I think is that the code to check the Baseline IC only checks for a SetElem_DenseAdd stub. This stub is only added when we are adding a new element immediately after the last one. ScriptAnalysis::arrayWriteHole is also set when there's a bunch of holes between the last element and the new one.

Using ScriptAnalysis is still wrong though, in most cases there's no ScriptAnalysis until Ion compilation and this can lead to immediate bailouts. Please file a follow-up bug to move this bit to the Baseline IC and set it from there.
Attachment #777476 - Flags: review?(jdemooij) → review+
Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/65fd27c97676

Filing the follow-up bug now.
Aaand, backed out again. For Dromaeo DOM-traversal regressions, this time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbfc4e1d054
https://hg.mozilla.org/mozilla-central/rev/65fd27c97676
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Not actually fixed, as, effectively, no effectively-dead code was found. :(
Resolution: FIXED → INVALID
Target Milestone: mozilla25 → ---
(In reply to Till Schneidereit [:till] from comment #10)
> Aaand, backed out again. For Dromaeo DOM-traversal regressions, this time.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbfc4e1d054

https://hg.mozilla.org/mozilla-central/rev/ccbfc4e1d054
You need to log in before you can comment on or make changes to this bug.