Closed Bug 1520473 Opened 10 months ago Closed 10 months ago

Global constant lexicals are always marked as overwritten properties by TI

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file)

var konst = "prop1" is compiled to:

00000:  defvar "konst"                  # 
main:
00005:  bindgname "konst"               # GLOBAL
00010:  string "prop1"                  # GLOBAL "prop1"
00015:  setgname "konst"                # "prop1"
00020:  pop                             # 

whereas const konst = "prop1" is compiled to:

00000:  defconst "konst"                # 
main:
00005:  string "prop1"                  # "prop1"
00010:  initglexical "konst"            # "prop1"
00015:  pop                             # 

JSOP_SETGNAME eventually ends up in NativeSetExistingDataProperty, which has this nice optimization:

      // Global properties declared with 'var' will be initially
      // defined with an undefined value, so don't treat the initial
      // assignments to such properties as overwrites.
      bool overwriting = !obj->is<GlobalObject>() ||
                         !obj->getSlot(shape->slot()).isUndefined();
      obj->setSlotWithType(cx, shape, v, overwriting);

JSOP_INITLEXICAL ends up in InitGlobalLexicalOperation, which calls setSlotWithType without setting overwriting to false:

lexicalEnv->setSlotWithType(cx, shape, value);

And that means TI treats the global lexical as an overwritten property.

Which in turn results in never taking this branch in Ion, but instead always loading global lexicals through slot loads.

We should try to mark global constant lexicals as non-overwritten properties, so that TI can treat them as constants, too.

Attached patch bug1520473.patchSplinter Review

This one seems to work. I've also added an assertion to ensure the slot is still uninitialised, just in case it's somehow possible with non-syntactic scopes to execute JSOP_INITGLEXICAL twice.

Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #9037236 - Flags: review?(jdemooij)
Comment on attachment 9037236 [details] [diff] [review]
bug1520473.patch

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

Good find. I can't think of any issues with this.
Attachment #9037236 - Flags: review?(jdemooij) → review+

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5579e90ade
Don't set the overwritten-bit in TI when initialising global lexicals. r=jandem

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.