Bug 198229 – JITOperations getByVal should mark negative array indices as out-of-bounds

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 370586 [details]
Patch

[Saam Barati]

Comment on attachment 370586 [details]
Patch

Nice. r=me

[Saam Barati]

Comment on attachment 370586 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370586&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        but baseline doesn't mark it as out-of-bounds, since it only considers positive indices. This
> +        leads to discarding DFG code, recompiling it and exiting at the same bytecode.

Can you quote where in JS2 you found this w/ the expected speedup?

[Tadeu Zagallo]

Created attachment 370630 [details]
Patch for landing

[Tadeu Zagallo]

I think I might have messed up the measurements, let me double-check it before landing.

[WebKit Commit Bot]

Comment on attachment 370630 [details]
Patch for landing

Clearing flags on attachment: 370630

Committed r245769: <https://trac.webkit.org/changeset/245769>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51134153>

[Saam Barati]

(In reply to Tadeu Zagallo from comment #5)
> I think I might have messed up the measurements, let me double-check it
> before landing.

I was interested in how much faster the individual test gets. IIRC, on IRC, you said something like 5%-10%. I wouldn’t expect 5% on an individual test to show up as a measurable overall progression. When I make changes like this, I like to quote the impact on both the individual JS2 test and on the microbenchmark

[Tadeu Zagallo]

(In reply to Saam Barati from comment #9)
> (In reply to Tadeu Zagallo from comment #5)
> > I think I might have messed up the measurements, let me double-check it
> > before landing.
> 
> I was interested in how much faster the individual test gets. IIRC, on IRC,
> you said something like 5%-10%. I wouldn’t expect 5% on an individual test
> to show up as a measurable overall progression. When I make changes like
> this, I like to quote the impact on both the individual JS2 test and on the
> microbenchmark

Oops, my bad, will keep that in mind going forward. FWIW, it looks like a ~10% progression in prepack-wtb (which I'm a bit skeptical of, since the function wasn't that hot) and the microbenchmark is ~4% faster, but is within the noise (maybe it wasn't a great microbenchmark after all...)

[Yusuke Suzuki]

Nice! Seems that on iOS devices it gets 1-3% improvement in JetStream2/uglify-js-wtb