JSFunction code clean-ups noted while working on the async wrapper removal
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(6 files, 6 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
Various clean-ups noted while working on bug 1530324.
Assignee | ||
Comment 1•2 years ago
|
||
Removes from JSFunction.h
- Unnecessary forward declaration for
JSAtomState
- Exported entries for
fun_symbolHasInstance
andfunction_methods
- Declaration for
function_selfhosted_methods
which is never defined.
But since C++ can't forward declare static arrays, function_methods
must also be rearranged in JSFunction.cpp to appear before JSFunctionClassSpec
. Sigh.
Assignee | ||
Comment 2•2 years ago
|
||
Adds a new helper to retrieve the default prototype for different function kinds.
Snatched Jason's comment from here to save me the extra work to document the new function. :-D
Assignee | ||
Comment 3•2 years ago
|
||
I saw that JSFunction::INTERPRETED_METHOD
is identical to JSFunction::INTERPRETED_METHOD_GENERATOR_OR_ASYNC
, so it seemed sensible to me to remove JSFunction::INTERPRETED_METHOD_GENERATOR_OR_ASYNC
. Then I looked at the other flags with the _GENERATOR_OR_ASYNC
suffix and wondered, "Doesn't it make more sense to add a _CONSTRUCTOR
suffix to interpreted, constructor functions than to use _GENERATOR_OR_ASYNC
for interpreted, non-constructor functions?!". Especially because that'd make interpreted function flags more consistent with the flags for native and asm.js functions, which both add a suffix to denote constructor functions.
So basically the following removals/renaming took place:
INTERPRETED_METHOD_GENERATOR_OR_ASYNC
→INTERPRETED_METHOD
INTERPRETED_LAMBDA_GENERATOR_OR_ASYNC
→INTERPRETED_LAMBDA
INTERPRETED_GENERATOR_OR_ASYNC
→INTERPRETED
INTERPRETED_LAMBDA
→INTERPRETED_LAMBDA_CONSTRUCTOR
INTERPRETED_NORMAL
→INTERPRETED_CONSTRUCTOR
Assignee | ||
Comment 4•2 years ago
|
||
The function body of the four compile-standalone functions were basically identically, save for selecting the generator and async kinds. Add a new helper function to reduce the code duplication.
(Alternatively we could also expose a single CompileStandaloneFunction
from BytecodeCompiler.h
which lets you directly select the generator and async kind, but that'd mean to include JSFunction.h
(and its transitive headers) there, too.)
Assignee | ||
Comment 5•2 years ago
|
||
JSOP_RESUME
's immediate was changed at some point from JOF_UINT8
to JOF_UINT16
, but as far as I can tell, that only happened because JSOP_RESUME
is emitted by BytecodeEmitter::emitCall
(and operations emitted from emitCall
must use a JOF_UINT16
immediate). So if we don't use emitCall
, we can change JSOP_RESUME
back to use a JOF_UINT8
immediate.
And while there added some static_assert
s in GeneratorObject which must hold for isAfterYieldOrAwait
.
Comment 6•2 years ago
|
||
Comment on attachment 9046775 [details] [diff] [review] bug1530745-part3-function-flags.patch Review of attachment 9046775 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure whether the new naming convention is intuitive or not. of course, technically normal function is constructor and async/generator are not, but calling async/generator INTERPRETED/INTERPRETED_LAMBDA sounds somewhat misleading, because they sound more normal/usual than INTERPRETED_CTOR/INTERPRETED_LAMBDA_CTOR, (IMO being non-ctor is more special), and also given INTERPRETED_LAMBDA has been different value. can they be called INTERPRETED_NON_CTOR/INTERPRETED_LAMBDA_NON_CTOR or something, to clarify what the relation between INTERPRETED_CTOR/INTERPRETED_LAMBDA_CTOR is? ::: js/src/vm/JSFunction.h @@ +108,3 @@ > INTERPRETED_LAMBDA_ARROW = INTERPRETED | LAMBDA | ARROW_KIND, > + INTERPRETED_LAMBDA_CONSTRUCTOR = INTERPRETED | LAMBDA | CONSTRUCTOR, > + INTERPRETED_CONSTRUCTOR = INTERPRETED | CONSTRUCTOR, INTERPRETED_LAMBDA_CTOR/INTERPRETED_CTOR maybe? to align with NATIVE_CTOR/ASMJS_CTOR.
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 7•2 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #6)
I'm not sure whether the new naming convention is intuitive or not.
I totally understand why you're probably a bit reserved about this change, I felt the same at first. Only after changing the names bit by bit I got more accustomed to the new meanings. :-)
Argh, and I forgot to document another reason why I went the changed names: The new names make it easier to see which base JSFunction flag combination was used to create the derived flags.
INTERPRETED
+METHOD_KIND
→INTERPRETED_METHOD
INTERPRETED
+CLASSCONSTRUCTOR_KIND
+CONSTRUCTOR
→INTERPRETED_CLASS_CONSTRUCTOR
(instead ofINTERPRETED_CLASS_CONSTRUCTOR_CONSTRUCTOR
to avoid duplicatingCONSTRUCTOR
)INTERPRETED
+GETTER_KIND
→INTERPRETED_GETTER
INTERPRETED
+SETTER_KIND
→INTERPRETED_SETTER
INTERPRETED
+LAMBDA
→INTERPRETED_LAMBDA
INTERPRETED
+LAMBDA
+ARROW_KIND
→INTERPRETED_LAMBDA_ARROW
INTERPRETED
+LAMBDA
+CONSTRUCTOR
→INTERPRETED_LAMBDA_CONSTRUCTOR
INTERPRETED
+CONSTRUCTOR
→INTERPRETED_CONSTRUCTOR
can they be called INTERPRETED_NON_CTOR/INTERPRETED_LAMBDA_NON_CTOR or something,
to clarify what the relation between INTERPRETED_CTOR/INTERPRETED_LAMBDA_CTOR is?
I guess we can do that for now to avoid possible misunderstandings. And maybe at a later date, when the new names have settled in, drop the NON_CTOR
suffix. :-D
Assignee | ||
Comment 8•2 years ago
|
||
It turned out part 2 doesn't quite work yet, because right now JSFunction::asyncKind
can only be called with interpreted functions. So I needed to change it to work more like JSFunction::generatorKind
which allows us to call it with native and selfhosted functions, too.
Assignee | ||
Comment 9•2 years ago
|
||
Comment on attachment 9046783 [details] [diff] [review] bug1530745-part5-resume-op-imm-length.patch It looks like some code somewhere relies on this being an uint16 immediate: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08cdcdc82cee3810d821d96c3d941a3784581de9 Unfortunately I wasn't able to find where exactly this code lives, so I'll just drop this patch for now. :-/
Updated•2 years ago
|
Comment 10•2 years ago
|
||
(In reply to André Bargull [:anba] from comment #7)
Argh, and I forgot to document another reason why I went the changed names: The new names make it easier to see which base JSFunction flag combination was used to create the derived flags.
INTERPRETED
+METHOD_KIND
→INTERPRETED_METHOD
INTERPRETED
+CLASSCONSTRUCTOR_KIND
+CONSTRUCTOR
→INTERPRETED_CLASS_CONSTRUCTOR
(instead ofINTERPRETED_CLASS_CONSTRUCTOR_CONSTRUCTOR
to avoid duplicatingCONSTRUCTOR
)INTERPRETED
+GETTER_KIND
→INTERPRETED_GETTER
INTERPRETED
+SETTER_KIND
→INTERPRETED_SETTER
INTERPRETED
+LAMBDA
→INTERPRETED_LAMBDA
INTERPRETED
+LAMBDA
+ARROW_KIND
→INTERPRETED_LAMBDA_ARROW
INTERPRETED
+LAMBDA
+CONSTRUCTOR
→INTERPRETED_LAMBDA_CONSTRUCTOR
INTERPRETED
+CONSTRUCTOR
→INTERPRETED_CONSTRUCTOR
I see, the convention itself is reasonable, given current base flags.
now I'm feeling that base flags need to be tweaked a bit.
what I'm now thinking is the following:
- flip
CONSTRUCTOR
intoNON_CONSTRUCTOR
- merge
NON_CONSTRUCTOR
andSELF_HOSTED
into FunctionKind- this way FunctionKind can take up to 5 bits (4 bits are enough for now)
- one bit can be used as
NON_CONSTRUCTOR
, forisConstructor()
check (including JIT code) NormalFunction
/ClassConstructor
/AsmJS
don't haveNON_CONSTRUCTOR
bitArrow
/Method
/Getter
/Setter
haveNON_CONSTRUCTOR
bit- add
NonConstructor
kind (for generator and async, and maybe some native/wasm?), that haveNON_CONSTRUCTOR
bit - add
SelfHosted
kind, that haveNON_CONSTRUCTOR
bit
- code that was checking
CONSTRUCTOR
can be rewritten to checkNON_CONSTRUCTOR
bit
like:
enum FunctionKind {
// NON_CONSTRUCTOR bit
// |
// v
NormalFunction = 0b0000,
ClassConstructor = 0b0001,
AsmJS = 0b0010,
NonConstructor = 0b1000,
SelfHosted = 0b1001,
Arrow = 0b1010,
Method = 0b1100,
Getter = 0b1101,
Setter = 0b1110
};
So that, function f() {}
becomes INTERPRETED
/INTERPRETED_LAMBDA
.
There are 2 arrow function in self-hosted code, but we can remove them, and disallow arrow function there.
Also, constructors in self-hosted code will have ClassConstructor
kind, not SelfHosted
kind, given SELF_HOSTED
flag is removed from it anyway.
one downside here is that the check for isSelfHosted()
needs to compare 4 bits, not 1 bit.
can they be called INTERPRETED_NON_CTOR/INTERPRETED_LAMBDA_NON_CTOR or something,
to clarify what the relation between INTERPRETED_CTOR/INTERPRETED_LAMBDA_CTOR is?I guess we can do that for now to avoid possible misunderstandings. And maybe at a later date, when the new names have settled in, drop the
NON_CTOR
suffix. :-D
If above sounds fine, can we go with NON_CTOR for now, and later move to the above path?
Comment 11•2 years ago
|
||
(In reply to André Bargull [:anba] from comment #9)
Comment on attachment 9046783 [details] [diff] [review]
bug1530745-part5-resume-op-imm-length.patchIt looks like some code somewhere relies on this being an uint16 immediate:
https://treeherder.mozilla.org/#/
jobs?repo=try&revision=08cdcdc82cee3810d821d96c3d941a3784581de9Unfortunately I wasn't able to find where exactly this code lives, so I'll
just drop this patch for now. :-/
needs to be something like following
if (*REGS.pc == JSOP_RESUME) {
ADVANCE_AND_DISPATCH(JSOP_RESUME_LENGTH);
} else {
ADVANCE_AND_DISPATCH(JSOP_CALL_LENGTH);
}
Assignee | ||
Comment 12•2 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #11)
Ah, thanks! I searched for JOF_UINT16
and JSOP_RESUME
uses, but not JOF_INVOKE
which may have led me to that spot.
Assignee | ||
Comment 13•2 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #10)
If above sounds fine, can we go with NON_CTOR for now, and later move to the above path?
Is it okay for you if I move that idea into a separate bug and reduce part 3 to only remove JSFunction::INTERPRETED_METHOD_GENERATOR_OR_ASYNC
in favour of keeping just JSFunction::INTERPRETED_METHOD
? Then we can first ask for more input from others about how they feel whether Constructors or Non-Constructors should be marked with a separate flag. (This would enable me to move this bug forward, so I can start fixing bug 1530754.)
Assignee | ||
Comment 14•2 years ago
|
||
Sigh, looks like a need to move everything from here to Phabricator to be able to request a new review for the minimised part 3.
Assignee | ||
Comment 15•2 years ago
|
||
Assignee | ||
Comment 16•2 years ago
|
||
Depends on D22665
Assignee | ||
Comment 17•2 years ago
|
||
Depends on D22667
Assignee | ||
Comment 18•2 years ago
|
||
Depends on D22668
Assignee | ||
Comment 19•2 years ago
|
||
Depends on D22669
Assignee | ||
Comment 20•2 years ago
|
||
And add more assertions to document implicit requirements about opcode lengths.
Depends on D22670
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 21•2 years ago
|
||
Moved everything from here to there, but r+ can't be carried forward, so new review requests for already reviewed patches are the result. Sorry about that! :-/
Comment 22•2 years ago
|
||
(In reply to André Bargull [:anba] from comment #13)
(In reply to Tooru Fujisawa [:arai] from comment #10)
If above sounds fine, can we go with NON_CTOR for now, and later move to the above path?
Is it okay for you if I move that idea into a separate bug and reduce part 3 to only remove
JSFunction::INTERPRETED_METHOD_GENERATOR_OR_ASYNC
in favour of keeping justJSFunction::INTERPRETED_METHOD
? Then we can first ask for more input from others about how they feel whether Constructors or Non-Constructors should be marked with a separate flag. (This would enable me to move this bug forward, so I can start fixing bug 1530754.)
Sure :)
Assignee | ||
Comment 23•2 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf70b25b1f892d3a1c3eea66f48b5e01349f0a3
Comment 24•2 years ago
|
||
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e4f92791cd3
Part 1: Make JSFunction::asyncKind() work with native and self-hosted functions. r=arai
https://hg.mozilla.org/integration/autoland/rev/4e633d85d45d
Part 2: Move declarations from JSFunction.h as static entries to JSFunction.cpp. r=arai
https://hg.mozilla.org/integration/autoland/rev/588b9eec2edb
Part 3: Add a helper to retrieve the prototype for a specific function type. r=arai
https://hg.mozilla.org/integration/autoland/rev/d6cab6cee202
Part 4: Remove INTERPRETED_METHOD_GENERATOR_OR_ASYNC in favour of using only INTERPRETED_METHOD. r=arai
https://hg.mozilla.org/integration/autoland/rev/b499a1eec1b1
Part 5: Move duplicate code for standalone function compilation into a separate function. r=arai
https://hg.mozilla.org/integration/autoland/rev/f6d4e1b012e6
Part 6: Change JSOP_RESUME immediate back to UINT8. r=arai
Comment 25•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e4f92791cd3
https://hg.mozilla.org/mozilla-central/rev/4e633d85d45d
https://hg.mozilla.org/mozilla-central/rev/588b9eec2edb
https://hg.mozilla.org/mozilla-central/rev/d6cab6cee202
https://hg.mozilla.org/mozilla-central/rev/b499a1eec1b1
https://hg.mozilla.org/mozilla-central/rev/f6d4e1b012e6
Description
•