Last Comment Bug 571617 - make constant-folding in the parser optional
: make constant-folding in the parser optional
Status: RESOLVED FIXED
reflect-parse fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Dave Herman [:dherman]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 659636 722087 (view as bug list)
Depends on: 533874
Blocks: 632026 632029 632056
  Show dependency treegraph
 
Reported: 2010-06-11 16:08 PDT by Dave Herman [:dherman]
Modified: 2012-08-19 20:54 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
template-izes js::Parser to make constant-folding optional (60.44 KB, patch)
2011-02-10 16:11 PST, Dave Herman [:dherman]
no flags Details | Diff | Splinter Review
dynamic flag, no template nonsense (5.01 KB, patch)
2011-06-07 00:06 PDT, Dave Herman [:dherman]
jorendorff: review+
Details | Diff | Splinter Review

Description Dave Herman [:dherman] 2010-06-11 16:08:33 PDT
It should be possible to turn off js_FoldConstants and FoldXMLConstants for the parser API, so users can get an AST that's closer to the actual input. This could easily be turned off with a boolean flag (which might not affect perf noticeably) or if necessary, templatization.

Dave
Comment 1 Chris Leary [:cdleary] (not checking bugmail) 2010-06-14 10:39:34 PDT
(In reply to comment #0)
> could easily be turned off with a boolean flag (which might not affect perf
> noticeably) or if necessary, templatization.

Definitely not going to affect perf if you're guarding a whole tree traversal with a single highly-predictable branch. (Correct me if I haven't understood correctly.)
Comment 2 Brendan Eich [:brendan] 2010-06-14 10:42:15 PDT
Emitter expects constant folder to have run, IIRC. So this would be for AST output only, at least without other changes.

/be
Comment 3 Dave Herman [:dherman] 2010-06-14 11:32:41 PDT
> Definitely not going to affect perf if you're guarding a whole tree traversal
> with a single highly-predictable branch. (Correct me if I haven't understood
> correctly.)

IIUC, the parser does recursively call the constant folder in the TOK_DELETE case, but I'd expect this to be rare enough not to matter.

Dave
Comment 4 Dave Herman [:dherman] 2010-06-14 11:33:20 PDT
> Emitter expects constant folder to have run, IIRC. So this would be for AST
> output only, at least without other changes.

Right.

Dave
Comment 5 Dave Herman [:dherman] 2011-02-10 16:11:12 PST
Created attachment 511556 [details] [diff] [review]
template-izes js::Parser to make constant-folding optional

Attaching an experimental patch that makes it possible to disable the use of js_FoldConstants. It leaves constant-folding on normally since (as mentioned in comment 2) it's required for the emitter. But it disables it for Reflect.parse.

The parser still performs constant-folding for very simple arithmetic expressions.

Dave
Comment 6 Dave Herman [:dherman] 2011-06-07 00:06:31 PDT
Created attachment 537743 [details] [diff] [review]
dynamic flag, no template nonsense

This patch is *vastly* simpler, using just a dynamic flag and no template silliness. It's pushed on top of a couple other pending patches in my hg queue so may not apply without failures. I'll try to land those other patches ASAP.

Dave
Comment 7 Jason Orendorff [:jorendorff] 2011-06-15 12:33:22 PDT
Comment on attachment 537743 [details] [diff] [review]
dynamic flag, no template nonsense

This wasn't r?anyone, but I'm for it! It needs a test, though. r=me with that.
Comment 8 Dave Herman [:dherman] 2011-06-18 08:46:57 PDT
http://hg.mozilla.org/tracemonkey/rev/9b37d1f464e5
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:13:08 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/9b37d1f464e5
Comment 10 Dave Herman [:dherman] 2012-08-19 20:52:58 PDT
*** Bug 722087 has been marked as a duplicate of this bug. ***
Comment 11 Dave Herman [:dherman] 2012-08-19 20:54:49 PDT
*** Bug 659636 has been marked as a duplicate of this bug. ***

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