Closed
Bug 1416793
Opened 7 years ago
Closed 7 years ago
Clean-up after legacy generator and comprehension removal
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(7 files, 3 obsolete files)
38.97 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
3.19 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
7.92 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
6.28 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
12.45 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
2.66 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
32.79 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Remove some remaining legacy generator and comprehension left-overs.
Assignee | ||
Comment 1•7 years ago
|
||
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
Attachment #8927862 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•7 years ago
|
||
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|).
Attachment #8927864 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•7 years ago
|
||
Removes unused FunctionBox::insideUseAsm field.
Attachment #8927865 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•7 years ago
|
||
Pack SharedContext more effectively (reduces sizeof from 40 bytes to 24 bytes).
Attachment #8927867 -
Flags: review?(jdemooij)
Comment 5•7 years ago
|
||
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"...
Attachment #8927862 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8927864 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8927865 -
Flags: review?(jdemooij) → review+
Comment 6•7 years ago
|
||
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!
Attachment #8927867 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 7•7 years ago
|
||
And I just saw that AnyContextFlags is only used in SharedContext, so we can simply inline it into SharedContext.
Attachment #8927887 -
Flags: review?(jdemooij)
Comment 8•7 years ago
|
||
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?
Attachment #8927887 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 9•7 years ago
|
||
(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?
Attachment #8928129 -
Flags: feedback?(jdemooij)
Comment 10•7 years ago
|
||
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.
Attachment #8928129 -
Flags: feedback?(jdemooij) → feedback+
Assignee | ||
Comment 11•7 years ago
|
||
(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.
Assignee | ||
Comment 12•7 years ago
|
||
Sort ParseContext's fields in increasing size to reduce sizeof from 504 to 496. Also remove no longer used |funHasReturnVoid| and |funHasReturnExpr| fields.
Attachment #8928178 -
Flags: review?(jdemooij)
Assignee | ||
Comment 13•7 years ago
|
||
FunctionContextFlags is only used in FunctionBox, so it probably makes sense to remove this extra indirection.
Attachment #8928182 -
Flags: review?(jdemooij)
Updated•7 years ago
|
Attachment #8928178 -
Flags: review?(jdemooij) → review+
Comment 14•7 years ago
|
||
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.
Attachment #8928182 -
Flags: review?(jdemooij) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8928129 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
Fix a nit in the commit message, otherwise no changes, therefore carrying r+.
Attachment #8927867 -
Attachment is obsolete: true
Attachment #8928506 -
Flags: review+
Assignee | ||
Comment 16•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7250b1ac7602efa60677265525b8e5b2714a2cdd
Keywords: checkin-needed
Comment 17•7 years ago
|
||
Hi :anba , the patches are failing to apply. Please update.
Flags: needinfo?(andrebargull)
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 18•7 years ago
|
||
Update part 1 to apply cleanly on inbound.
Attachment #8927862 -
Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #8928907 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 19•7 years ago
|
||
Pushed by ncsoregi@mozilla.com: 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
Keywords: checkin-needed
Comment 20•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•