1530745 - JSFunction code clean-ups noted while working on the async wrapper removal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Various clean-ups noted while working on bug 1530324.

Comment 1

[André Bargull [:anba]]

Attachment: bug1530745-part1-slim-header.patch

Removes from JSFunction.h

• Unnecessary forward declaration for JSAtomState
• Exported entries for fun_symbolHasInstance and function_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.

Comment 2

[André Bargull [:anba]]

Attachment: bug1530745-part2-default-funtype-proto.patch

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

Comment 3

[André Bargull [:anba]]

Attachment: bug1530745-part3-function-flags.patch

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

Comment 4

[André Bargull [:anba]]

Attachment: bug1530745-part4-compile-standalone.patch

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.)

Comment 5

[André Bargull [:anba]]

Attachment: bug1530745-part5-resume-op-imm-length.patch

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_asserts in GeneratorObject which must hold for isAfterYieldOrAwait.

Comment 6

[Tooru Fujisawa [:arai]]

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.

Comment 7

[André Bargull [:anba]]

(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 of INTERPRETED_CLASS_CONSTRUCTOR_CONSTRUCTOR to avoid duplicating CONSTRUCTOR)
• 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

Comment 8

[André Bargull [:anba]]

Attachment: bug1530745-part0-async-native-or-selfhosted.patch

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.

Comment 9

[André Bargull [:anba]]

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. :-/

Comment 10

[Tooru Fujisawa [:arai]]

(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 of INTERPRETED_CLASS_CONSTRUCTOR_CONSTRUCTOR to avoid duplicating CONSTRUCTOR)
• 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 into NON_CONSTRUCTOR
• merge NON_CONSTRUCTOR and SELF_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, for isConstructor() check (including JIT code)
  • NormalFunction/ClassConstructor/AsmJS don't have NON_CONSTRUCTOR bit
  • Arrow/Method/Getter/Setter have NON_CONSTRUCTOR bit
  • add NonConstructor kind (for generator and async, and maybe some native/wasm?), that have NON_CONSTRUCTOR bit
  • add SelfHosted kind, that have NON_CONSTRUCTOR bit
• code that was checking CONSTRUCTOR can be rewritten to check NON_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

[Tooru Fujisawa [:arai]]

(In reply to André Bargull [:anba] from comment #9)

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. :-/

that's here
https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/js/src/vm/Interpreter.cpp#2112

needs to be something like following

          if (*REGS.pc == JSOP_RESUME) {
            ADVANCE_AND_DISPATCH(JSOP_RESUME_LENGTH);
          } else {
            ADVANCE_AND_DISPATCH(JSOP_CALL_LENGTH);
          }

Comment 12

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #11)

that's here
https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/js/src/vm/Interpreter.cpp#2112

Ah, thanks! I searched for JOF_UINT16 and JSOP_RESUME uses, but not JOF_INVOKE which may have led me to that spot.

Comment 13

[André Bargull [:anba]]

(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.)

Comment 14

[André Bargull [:anba]]

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.

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1530745 - Part 1: Make JSFunction::asyncKind() work with native and self-hosted functions. r=arai!

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1530745 - Part 2: Move declarations from JSFunction.h as static entries to JSFunction.cpp. r=arai!

Depends on D22665

Comment 17

[André Bargull [:anba]]

Attachment: Bug 1530745 - Part 3: Add a helper to retrieve the prototype for a specific function type. r=arai!

Depends on D22667

Comment 18

[André Bargull [:anba]]

Attachment: Bug 1530745 - Part 4: Remove INTERPRETED_METHOD_GENERATOR_OR_ASYNC in favour of using only INTERPRETED_METHOD. r=arai!

Depends on D22668

Comment 19

[André Bargull [:anba]]

Attachment: Bug 1530745 - Part 5: Move duplicate code for standalone function compilation into a separate function. r=arai!

Depends on D22669

Comment 20

[André Bargull [:anba]]

Attachment: Bug 1530745 - Part 6: Change JSOP_RESUME immediate back to UINT8. r=arai!

And add more assertions to document implicit requirements about opcode lengths.

Depends on D22670

Comment 21

[André Bargull [:anba]]

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

[Tooru Fujisawa [:arai]]

(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 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.)

Sure :)

Comment 23

[André Bargull [:anba]]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

[Dorel Luca [:dluca]]

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