1416793 - Clean-up after legacy generator and comprehension removal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Remove some remaining legacy generator and comprehension left-overs.

Comment 1

[André Bargull [:anba]]

Attachment: bug1416793-part1-remove-leftovers.patch

Remove leftovers from comprehensions:

- Update/Remove comments mentioning comprehensions.
- Remove unused BytecodeEmitter::EmitterScope::enterComprehensionFor
- Remove unused {Full,Syntax}ParseHandler::isUnparenthesizedCommaExpression
- Remove unused SyntaxParseHandler::Node::NodeUnparenthesizedCommaExpr
- Remove no longer used error messages
- Remove unused entries from jsast.tbl
- Remove no longer used functions from PatternBuilders.js and alternateBuilder.js

Comment 2

[André Bargull [:anba]]

Attachment: bug1416793-part2-store-generator-async-kind.patch

A bit of clean-up to store generator and async information more consistently in various classes:

- Makes FunctionAsyncKind an enum class to match GeneratorKind.
- Changes FunctionBox::{generatorKind_, asyncKindBits_} to bit-fields FunctionBox::{isGenerator_, isAsync_} (needs to be |bool| instead of GeneratorKind/FunctionAsyncKind because of limitations in MSVC).
- And changes some accessors to test for generator and async kind more uniformly when performed in the same function (for example js::XDRScript(...) in jsscript.cpp tested for generators with |script->isGenerator()|, but async functions with |script->asyncKind() == AsyncFunction|).

Comment 3

[André Bargull [:anba]]

Attachment: bug1416793-part3-remove-insideUseAsm.patch

Removes unused FunctionBox::insideUseAsm field.

Comment 4

[André Bargull [:anba]]

Attachment: bug1416793-part4-pack-SharedContext.patch

Pack SharedContext more effectively (reduces sizeof from 40 bytes to 24 bytes).

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8927862 [details] [diff] [review]
bug1416793-part1-remove-leftovers.patch

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

Thanks! I forgot to do a search for "comprehension"...

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8927867 [details] [diff] [review]
bug1416793-part4-pack-SharedContext.patch

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

Nice, that's a big improvement!

Comment 7

[André Bargull [:anba]]

Attachment: bug1416793-part5-inline-AnyContextFlags.patch

And I just saw that AnyContextFlags is only used in SharedContext, so we can simply inline it into SharedContext.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8927887 [details] [diff] [review]
bug1416793-part5-inline-AnyContextFlags.patch

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

\o/

::: js/src/frontend/SharedContext.h
@@ -222,5 @@
>   */
>  class SharedContext
>  {
>    public:
>      JSContext* const context;

I wonder if we could remove this field too. It's used in the parser but there we could just (and more efficiently I think!) use ParserBase::context. Same for the bytecode emitter. Want to file a follow-up bug?

Comment 9

[André Bargull [:anba]]

Attachment: bug1416793-part6-remove-jscontext-from-sharedcontext.patch

(In reply to Jan de Mooij [:jandem] from comment #8)
> >  class SharedContext
> >  {
> >    public:
> >      JSContext* const context;
> 
> I wonder if we could remove this field too. It's used in the parser but
> there we could just (and more efficiently I think!) use ParserBase::context.
> Same for the bytecode emitter. Want to file a follow-up bug?

It's doable, but it required a few additional changes:

- BytecodeEmitter initializes its JSContext* from SharedContext, so we additionally need to pass JSContext* to the BytecodeEmitter constructor if SharedContext no longer has JSContext*. 
- Similar to that, ParseContext now needs to have its own JSContext* field.
- And a couple of functions now require an explicit JSContext* argument.

Do you think it's still worth it to remove the JSContext* field from SharedContext even with these additional changes?

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8928129 [details] [diff] [review]
bug1416793-part6-remove-jscontext-from-sharedcontext.patch

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

Thanks for trying this!

> Do you think it's still worth it to remove the JSContext* field from SharedContext even with these additional changes?

FunctionBox (inherits from SharedContext) is bigger than I thought it was so it probably doesn't win us that much.. If you want to land this I would be happy to review it though.

Comment 11

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> FunctionBox (inherits from SharedContext) is bigger than I thought it was so
> it probably doesn't win us that much.. If you want to land this I would be
> happy to review it though.

I wonder if it's more useful not to apply this patch, because it'll increase ParseContext by another 8 bytes (and |sizeof(ParseContext)| is already 504! [1]) and since ParseContext is allocated on the stack, increasing its size could be more harmful than decreasing the size of (the heap-allocated) FunctionBox from 128 to 120.

[1] ParseContext's size can be decreased to 496 by reordering the fields, I'll prepare a new patch to do that.

Comment 12

[André Bargull [:anba]]

Attachment: bug1416793-part6-pack-ParseContext.patch

Sort ParseContext's fields in increasing size to reduce sizeof from 504 to 496. Also remove no longer used |funHasReturnVoid| and |funHasReturnExpr| fields.

Comment 13

[André Bargull [:anba]]

Attachment: bug1416793-part7-inline-FunctionContextFlags.patch

FunctionContextFlags is only used in FunctionBox, so it probably makes sense to remove this extra indirection.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8928182 [details] [diff] [review]
bug1416793-part7-inline-FunctionContextFlags.patch

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

Nice, this is a lot simpler.

Comment 15

[André Bargull [:anba]]

Attachment: bug1416793-part4-pack-SharedContext.patch

Fix a nit in the commit message, otherwise no changes, therefore carrying r+.

Comment 16

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7250b1ac7602efa60677265525b8e5b2714a2cdd

Comment 17

[Narcis Beleuzu [:NarcisB]]

Hi :anba , the patches are failing to apply.
Please update.

Comment 18

[André Bargull [:anba]]

Attachment: bug1416793-part1-remove-leftovers.patch

Update part 1 to apply cleanly on inbound.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d31d1c23996
Part 1: Remove array and generator comprehension leftovers. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e15d070b06
Part 2: Make generator and async kind storage more consistent in various classes. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a280a0d1293b
Part 3: Remove unused insideUseAsm field in FunctionBox. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc85ce388c46
Part 4: Pack SharedContext more efficiently. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/00f2c8de7cce
Part 5: Inline AnyContextFlags into SharedContext. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d9f53bc8c9
Part 6: Pack ParseContext more efficiently. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fdf13f2118
Part 7: Inline FunctionContextFlags into FunctionBox. r=jandem

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5d31d1c23996
https://hg.mozilla.org/mozilla-central/rev/19e15d070b06
https://hg.mozilla.org/mozilla-central/rev/a280a0d1293b
https://hg.mozilla.org/mozilla-central/rev/fc85ce388c46
https://hg.mozilla.org/mozilla-central/rev/00f2c8de7cce
https://hg.mozilla.org/mozilla-central/rev/c2d9f53bc8c9
https://hg.mozilla.org/mozilla-central/rev/96fdf13f2118