Closed Bug 784295 Opened 7 years ago Closed 6 years ago

Investigate activating ES5 strict mode for all self-hosted JS

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: till, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:t][qa-])

Attachments

(2 files, 2 obsolete files)

Currently, self-hosted builtins aren't compiled in strict mode because doing so would impose a severe performance hit.

Bug 782579 contains a benchmark that exhibits a performance hit from activating strict mode, too, so investigating and solving that will probably go a long way towards solving the issues for self-hosting.
Whiteboard: [js:t]
Quick update: correctness-wise, there's nothing holding this back. All tests and jit-tests pass, at least.

I'm not attaching a patch, though, because I didn't do any benchmarking at all, yet, and we don't need to regress perf rather just for the fun of it.
Till: can we enable strict mode for builtins in debug builds? Firefox enables strict mode for chrome JS in debug builds. See bug 385872.

https://hg.mozilla.org/mozilla-central/file/febfe3c7732b/dom/base/nsJSEnvironment.cpp#l800
Flags: needinfo?(till)
Attached patch 784295-part-1-strict-mode.patch (obsolete) — Splinter Review
Part 1: Compile self-hosted JS in strict mode (in DEBUG builds).

Till: If you are concerned about performance regressions from strict mode, what do you think about enabling strict mode for builtins only in debug builds?

https://tbpl.mozilla.org/?tree=Try&rev=f76bd8fabc39
Attachment #8338304 - Flags: feedback?(till)
Flags: needinfo?(till)
Part 2: Compile self-hosted JS in Gecko's "extra warnings" mode (in DEBUG builds).

In bug 385872, Firefox enabled extra warnings mode for chrome JS in debug builds. (In comment 2 above, I mistakenly said Firefox enabled "strict mode" for chrome JS, but it actually enables Gecko's "strict warnings" AKA "extra warnings".)

To compile the self-hosted builtins in "extra warnings" mode, this patch:

* Renames duplicate function names (but not duplicate implementations!) in Array.js and ParallelArray.js. These duplicated function names are remnants of a partial back out of bug 860965.

* Renames ParallelArrayConstructFromArray's parameter name to avoid "redefined variable `buffer`" warning and match the comments.

* Adds `return undefined` statements to the end of some Array.js functions that define local functions after the function's (real) last return statement. A better fix would be to hoist the local function definitions, but that would make a bigger diff and the original author clearly preferred a coding style that "buried" the local function definitions at the end of the function. An even better fix would be to fix extra warnings mode to recognize this coding style.

https://tbpl.mozilla.org/?tree=Try&rev=911c69bbb324
Attachment #8338309 - Flags: feedback?(till)
Comment on attachment 8338309 [details] [diff] [review]
784295-part-2-extra-warnings.patch

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

Sorry for the long delay here!

I agree that we should enable this for debug builds. I wasn't convinced it would gain us much, until I saw the huge mess with the PJS functions. This really should never have happened, and probably causes subtle weird issues already.

However, I think we should clean up that situation instead of papering it over by renaming one set of these functions. Instead, they should be removed properly before landing this patch, so I'm clearing the review based on that.

I'll talk to nmatsakis about the timing of removing ParallelArray altogether. Depending on whether that'll happen soon or not, we can either wait for it, or do cleanup independently.
Attachment #8338309 - Flags: feedback?(till)
Comment on attachment 8338304 [details] [diff] [review]
784295-part-1-strict-mode.patch

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

thanks!
Attachment #8338304 - Flags: feedback?(till) → feedback+
Part 1: Compile self-hosted JS in strict mode (in DEBUG builds).

Also add `var` to top-level X4LaneStrings definition in TypedObject.js. The code appears to define a new property on the global object, which AFAIK should be a strict mode error though it did not throw a ReferenceError in strict mode.
Assignee: general → cpeterson
Attachment #8338304 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8340679 - Flags: review?(till)
Part 2: Compile self-hosted JS in extra warnings mode (in DEBUG builds).

To address the feedback in comment 5, I removed the ParallelArray.js functions that duplicate Array.js functions. 

Some ParallelArray.js functions have the same names but different implementations as Array.js functions because they were optimized in bug 906773. For those functions, I kept the ParallelArray.js implementations and moved them to Array.js.

AFAICT, extra warnings mode's checks should not affect runtime performance. I did not see performance regressions from extra warnings mode or strict mode, but I only set these flags #ifdef DEBUG because I don't want to be the person responsible for regressing performance. <:)
Attachment #8338309 - Attachment is obsolete: true
Attachment #8340680 - Flags: review?(till)
Comment on attachment 8340679 [details] [diff] [review]
part-1-strict-mode-v2.patch

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

::: js/src/builtin/TypedObject.js
@@ +663,5 @@
>    }
>    assert(false, "Unhandled type constant");
>  }
>  
> +var X4LaneStrings = ["x", "y", "z", "w"];

hrmpf, I should've seen this in my review.
Attachment #8340679 - Flags: review?(till) → review+
Comment on attachment 8340680 [details] [diff] [review]
part-2-extra-warnings-v2.patch

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

Thank you for doing this! I asked nmatsakis and shu on IRC and they think that we can outright remove ParallelArray now. So you can either land this as-is, or wait for shu's patch doing just that to land. Sorry about not posting that info here. :(
Attachment #8340680 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/c399711f3658
https://hg.mozilla.org/mozilla-central/rev/763fbb0e1c45
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [js:t] → [js:t][qa-]
Blocks: 995200
Depends on: 1053061
You need to log in before you can comment on or make changes to this bug.