Closed Bug 899296 Opened 6 years ago Closed 6 years ago

Frontend: Parser no longer desugars foo["ab"] to foo.ab

Categories

(Core :: JavaScript Engine, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- unaffected
firefox25 + fixed
firefox26 --- fixed

People

(Reporter: efaust, Assigned: jorendorff)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file Testcase
Test case above yields

00000:  this
00001:  string "ab"
00006:  getelem
00007:  pop
00008:  this
00009:  string "a"
00014:  string "b"
00019:  add
00020:  getelem
00021:  pop
00022:  stop

on inbound tip.
Seems to be OK on Aurora 24, so nightly-only regression so far.
Keywords: regression
Bisect says:

The first bad revision is:
changeset:   137884:32c80f595de5
user:        Jason Orendorff <jorendorff@mozilla.com>
date:        Wed Jul 10 08:14:02 2013 -0500
summary:     Bug 844805, part 3 - Remove a call to FoldConstants from Parser::memberExpr. r=Waldo.
Blocks: 844805
Note that that's the regressing changeset for both the "ab" and "a" + "b" versions of the test...
This is interesting.  Consider this testcase:

  var x = 'obj["a" + "b"]; obj["cd"];';
  var f = new Function(x);
  dis(f);

this outputs:

  main:
  00000:  getgname "obj"
  00005:  getprop "ab"
  00010:  pop
  00011:  getgname "obj"
  00016:  getprop "cd"
  00021:  pop
  00022:  stop

but an actual function expression being parsed normally ends up with getelem calls!
The PNK_ELEM case in Fold() that bug 844805 added is not reached at all for the normal function case, as far as I can tell.

If I turn off lazy parsing, then I get the bytecode I expect getting emitted.

I don't see frontend::CompileLazyFunction doing any FoldConstants.  Should it be?
Er, I suppose with null() instead of NULL.
The first bad revision is:
changeset:   137884:32c80f595de5
user:        Jason Orendorff <jorendorff@mozilla.com>
date:        Wed Jul 10 08:14:02 2013 -0500
summary:     Bug 844805, part 3 - Remove a call to FoldConstants from Parser::memberExpr. r=Waldo.

Darn it.
Assignee: general → jorendorff
Comment on attachment 782913 [details] [diff] [review]
Is this at all a sane thing to do?

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

Clearing feedback flag.

This is the right idea, but the actual FoldConstants call should go in BytecodeCompiler.cpp, in the interest of consistency.

These three entry points to the compiler should each contain one FoldConstants call:
- frontend::CompileScript
- frontend::CompileLazyFunction
- frontend::CompileFunctionBody

...and ideally, nothing else should, parsing and constant folding being two separate passes. Though we're stuck with one or two calls from the parser for the time being.

I need to look at it some more.
Attachment #782913 - Flags: feedback?(jorendorff)
For what it's worth, I tried putting the FoldConstants in CompileLazyFunction initially.  It crashes with a null-deref because the ParseContext that the parser used in standaloneLazyFunction() has gone out of scope so parser.context is null.

I agree that it would be nice to not have that dependency.
Tracking as a regression in 25.
Comment on attachment 782913 [details] [diff] [review]
Is this at all a sane thing to do?

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

Try Servering this patch.
Attachment #782913 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4473d84c6ab6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 782913 [details] [diff] [review]
Is this at all a sane thing to do?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 844805
User impact if declined:
  JavaScript performance regression going from FF24 to FF25.
Testing completed (on m-c, etc.): Landed in m-c 4 days ago.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.
Attachment #782913 - Flags: approval-mozilla-beta?
Attachment #782913 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm still getting daily tracking emails about this, but I thought it was done. Please let me know what remains here, or clear the bit.
Depends on: 937697
You need to log in before you can comment on or make changes to this bug.