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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(6 attachments, 6 obsolete attachments)

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
Assignee

Description

4 months ago

Various clean-ups noted while working on bug 1530324.

Assignee

Comment 1

4 months ago

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.

Attachment #9046769 - Flags: review?(arai.unmht)
Assignee

Comment 2

4 months 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

Attachment #9046772 - Flags: review?(arai.unmht)
Assignee

Comment 3

4 months 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_ASYNCINTERPRETED_METHOD
  • INTERPRETED_LAMBDA_GENERATOR_OR_ASYNCINTERPRETED_LAMBDA
  • INTERPRETED_GENERATOR_OR_ASYNCINTERPRETED
  • INTERPRETED_LAMBDAINTERPRETED_LAMBDA_CONSTRUCTOR
  • INTERPRETED_NORMALINTERPRETED_CONSTRUCTOR
Attachment #9046775 - Flags: review?(arai.unmht)
Assignee

Comment 4

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

Attachment #9046777 - Flags: review?(arai.unmht)
Assignee

Comment 5

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

Attachment #9046783 - Flags: review?(arai.unmht)
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.
Attachment #9046775 - Flags: review?(arai.unmht) → feedback+
Attachment #9046772 - Flags: review?(arai.unmht) → review+
Attachment #9046769 - Flags: review?(arai.unmht) → review+
Attachment #9046777 - Flags: review?(arai.unmht) → review+
Attachment #9046783 - Flags: review?(arai.unmht) → review+
Assignee

Comment 7

4 months 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_KINDINTERPRETED_METHOD
  • INTERPRETED + CLASSCONSTRUCTOR_KIND + CONSTRUCTORINTERPRETED_CLASS_CONSTRUCTOR (instead of INTERPRETED_CLASS_CONSTRUCTOR_CONSTRUCTOR to avoid duplicating CONSTRUCTOR)
  • INTERPRETED + GETTER_KINDINTERPRETED_GETTER
  • INTERPRETED + SETTER_KINDINTERPRETED_SETTER
  • INTERPRETED + LAMBDAINTERPRETED_LAMBDA
  • INTERPRETED + LAMBDA + ARROW_KINDINTERPRETED_LAMBDA_ARROW
  • INTERPRETED + LAMBDA + CONSTRUCTORINTERPRETED_LAMBDA_CONSTRUCTOR
  • INTERPRETED + CONSTRUCTORINTERPRETED_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

4 months 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.

Attachment #9047086 - Flags: review?(arai.unmht)
Assignee

Comment 9

4 months 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. :-/
Attachment #9046783 - Attachment is obsolete: true
Attachment #9047086 - Flags: review?(arai.unmht) → review+

(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_KINDINTERPRETED_METHOD
  • INTERPRETED + CLASSCONSTRUCTOR_KIND + CONSTRUCTORINTERPRETED_CLASS_CONSTRUCTOR (instead of INTERPRETED_CLASS_CONSTRUCTOR_CONSTRUCTOR to avoid duplicating CONSTRUCTOR)
  • INTERPRETED + GETTER_KINDINTERPRETED_GETTER
  • INTERPRETED + SETTER_KINDINTERPRETED_SETTER
  • INTERPRETED + LAMBDAINTERPRETED_LAMBDA
  • INTERPRETED + LAMBDA + ARROW_KINDINTERPRETED_LAMBDA_ARROW
  • INTERPRETED + LAMBDA + CONSTRUCTORINTERPRETED_LAMBDA_CONSTRUCTOR
  • INTERPRETED + CONSTRUCTORINTERPRETED_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?

(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);
          }
Assignee

Comment 12

3 months ago

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

Assignee

Comment 13

3 months 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

3 months 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 20

3 months ago

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

Depends on D22670

Assignee

Updated

3 months ago
Attachment #9046769 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9046772 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9046775 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9046777 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9047086 - Attachment is obsolete: true
Assignee

Comment 21

3 months 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! :-/

(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 24

3 months 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

Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.