894948 - Remove effectively-dead analysis-related code from interpreter

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

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.

Comment 1

[Till Schneidereit [:till]]

Attachment: Remove effectively-dead analysis-related code from interpreter

I tested SunSpider 1.0 and v8-v6: this has no influence on perf whatsoever.

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95a4b87a0583

5 files changed, 4 insertions(+), 81 deletions(-)

Comment 4

[Luke Wagner [:luke]]

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

Comment 5

[Till Schneidereit [:till]]

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

Comment 6

[Till Schneidereit [:till]]

Attachment: Backout of the removal of ScriptAnalysis::arrayWriteHole

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!

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

[Jan de Mooij [:jandem]]

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.

Comment 9

[Till Schneidereit [:till]]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/65fd27c97676

Filing the follow-up bug now.

Comment 10

[Till Schneidereit [:till]]

Aaand, backed out again. For Dromaeo DOM-traversal regressions, this time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbfc4e1d054

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65fd27c97676

Comment 12

[Till Schneidereit [:till]]

Not actually fixed, as, effectively, no effectively-dead code was found. :(

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(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