Last Comment Bug 1328077 - CacheIR: Attach DenseElementHole even with zero dense elements
: CacheIR: Attach DenseElementHole even with zero dense elements
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine: JIT (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal with 1 vote (vote)
: mozilla53
Assigned To: Tom Schuster [:evilpie]
:
: Hannes Verschore [:h4writer]
Mentors:
Depends on:
Blocks: CacheIR
  Show dependency treegraph
 
Reported: 2017-01-01 05:02 PST by Tom Schuster [:evilpie]
Modified: 2017-01-04 18:50 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
WIP (1.59 KB, patch)
2017-01-01 05:02 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v1 Attach dense element hole IC even with zero dense elements (2.08 KB, patch)
2017-01-03 13:29 PST, Tom Schuster [:evilpie]
jdemooij: review+
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Tom Schuster [:evilpie] 2017-01-01 05:02:18 PST
Created attachment 8822943 [details] [diff] [review]
WIP

Found on twitter again. As long as the object doesn't have indexed properties besides dense elements we should be able to attach this IC even when the initialized length is zero. This basically means there are no indexed elements anywhere on that object.
Comment 1 User image Tom Schuster [:evilpie] 2017-01-02 13:05:38 PST
Apparently there is some edge case where this is causing issues: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff705046fbbb21cd440eeb6effc09f4b273fddf9&selectedJob=65747940. Just started debugging.
Comment 2 User image Tom Schuster [:evilpie] 2017-01-02 13:45:18 PST
For some reason we are attaching the stub for CData objects (http://searchfox.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#583) which should be impossible, because that class has a getProperty ClassOps, which we check for in ClassCanHaveExtraProperties. Compiler bug?
Comment 3 User image Tom Schuster [:evilpie] 2017-01-03 13:29:32 PST
Created attachment 8823421 [details] [diff] [review]
v1 Attach dense element hole IC even with zero dense elements

Somehow I didn't spot the difference between getProperty in ClassOps and ObjectOps. 

Anyway I am asking Brian for additional review. What we are trying to accomplish here is make sure that indexed properties would either be defined as a dense element or not exist at all. This also means if the object currently has zero dense elements we have to make sure there is no other way for indexed properties to appear.
Comment 4 User image Jan de Mooij [:jandem] 2017-01-04 03:22:00 PST
Comment on attachment 8823421 [details] [diff] [review]
v1 Attach dense element hole IC even with zero dense elements

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

LGTM.
Comment 5 User image Pulsebot 2017-01-04 13:12:27 PST
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f897399fb28a
Attach dense element hole IC even with zero dense elements. r=jandem,bhackett
Comment 6 User image Guilherme Lima 2017-01-04 18:16:16 PST
This patch caused a 40% improvement in https://arewefastyet.com/#machine=29&view=single&suite=misc&subtest=bugs-636096-model2d
Comment 7 User image Phil Ringnalda (:philor) 2017-01-04 18:50:12 PST
https://hg.mozilla.org/mozilla-central/rev/f897399fb28a

Note You need to log in before you can comment on or make changes to this bug.