902383 - Remove left over analysis stuff in interpreter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

I was digging more into type analysis when encounter this code. I remember we only start generating the analysis info when we hit the baseline. So checking hasAnalysis in the interpreter should always return false.  In other words this is dead code, IIRC.

Time to clean.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bug902383-remove-leftovers

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: bug902383-remove-leftovers-p2

One-liner, removing getStringElement too. Looks like we never use that information and we now don't set either. So goodbye.

Comment 3

[Till Schneidereit [:till]]

I tried removing some analysis code in bug 894948, and failed pretty hard, because it regressed several benchmarks. So I'd recommend carefully measuring the impact and making sure that no regressions occur.

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Till Schneidereit [:till] from comment #3)
> I tried removing some analysis code in bug 894948, and failed pretty hard,
> because it regressed several benchmarks. So I'd recommend carefully
> measuring the impact and making sure that no regressions occur.

I just looked to your patch. It is almost the same as this one, but there is a big fault in IonBuilder. You were probably a bit to eager. I think that caused the regression!

Comment 5

[Till Schneidereit [:till]]

I'm absolutely sure that someone who actually knows much about this stuff can do a lot better than me. :) Just wanted to make you aware of potential pitfalls.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 786829 [details] [diff] [review]
bug902383-remove-leftovers

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

::: js/src/vm/Interpreter.cpp
@@ +1265,5 @@
>          if ((uint32_t)i >= length) {
> +            // Annotate script if provided with information (e.g. baseline)
> +            if (script && script->hasAnalysis()) {
> +                JS_ASSERT(pc);
> +                script->analysis()->getCode(pc).arrayWriteHole = true;

For most scripts, ScriptAnalysis is only allocated once we Ion-compile (Baseline has its own bytecode analysis now that's freed immediately after compilation). So this could trigger unnecessary bailouts because most of the time script->hasAnalysis() will be false.

Would you mind filing a new bug/patch to remove this code as well, and have DoSetElemFallback set a bit on the baseline IC instead? See also the stub->noteNonNativeAccess() call there.

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd4230f5809

(In reply to Jan de Mooij [:jandem] from comment #6)
> Would you mind filing a new bug/patch to remove this code as well, and have
> DoSetElemFallback set a bit on the baseline IC instead? See also the
> stub->noteNonNativeAccess() call there.

Till already opened one: bug 895332

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1cd4230f5809