SetFunctionNameIfNoOwnName currently always calls the resolve-hook and defines a new slot for the "name" property. We should try to avoid both calls, maybe we can simply stash the function name in the existing JSFunction::atom_ field? This µ-benchmark improved from 760ms to 130ms when I simply stored the function name in JSFunction::atom_: var t = Date.now(); for (var i=0;i<1000000;++i) { var o = {["a"]: function (){}}; } print(Date.now() - t);

First some clean-ups and simplifications: Parser.cpp, SyntaxParseHandler.h, FullParseHandler.h, BytecodeEmitter.cpp: - Only call checkAndSetIsDirectRHSAnonFunction() if the left-hand side is definitely an unparenthesised identifier. That way we don't need to check this condition in the BytecodeEmitter and it will simplify a change for part 3. BytecodeEmitter::setOrEmitSetFunName() - Add an extra assertion for clarity that this function should only be called if the function/class is marked as |isDirectRHSAnonFunction()|. NameResolver::resolve(): - Add null-pointer checks for ParseNodeKind::Name and ParseNodeKind::Function, so we can remove the |cur == nullptr| check at the top of resolve(). All other callers already check for null-pointers before recursively calling resolve(). - Update the checks for ParseNodeKind::For{In,Of} (pn_kid1 is never a null-pointer, pn_kid2 is always a null-pointer). - Update some comments and fix indentation.

- Remove a straight out lie in the IdToFunctionName() doc-comment. - Change SetFunctionNameIfNoOwnName to skip the Value->Id conversion and instead directly construct the function name from a Symbol or Atom value. - And replace the HasOwnProperty() call with |NativeObject::contains()|, so we don't run the resolve-handler for class constructors.

The actual change to store the function name in JSFunction::atom_ to avoid creating a "name" property: - Replace |CompileTimeName| with |InferredName| in various methods/fields because we no longer only set the inferred name at compile-time. - Remove the empty name assertion in JSFunction::getUnresolvedName(), because this case is now possible: |var o = {[""]: class {}};|. - Remove HAS_COMPILE_TIME_NAME/HAS_INFERRED_NAME from JSFunction::STABLE_ACROSS_CLONES, because it may not be set in class constructor functions: |var c = class {static [randomOneOf("name", "no-name")](){}};|. (It's correct to remove it because of that case, right?) - Don't assign a guessed function name in NameResolve::resolveFun() if we know that this function will get a (dynamically computed) inferred name at runtime. And now most important: Do you think this change is correct or is there some subtle reason why we can't save the function name in JSFunction::atom_ for dynamically computed names?

Comment on attachment 8960205 [details] [diff] [review] bug1371591-part1-mark-anon-in-parser.patch Review of attachment 8960205 [details] [diff] [review]: ----------------------------------------------------------------- r=me and I sure hope these comments are helpful! ::: js/src/frontend/BytecodeEmitter.cpp @@ +5488,1 @@ > if (maybeFun->isKind(ParseNodeKind::Function)) { The only other possibility is ParseNodeKind::Class, right? Would you mind asserting that after this if-block? ::: js/src/frontend/FullParseHandler.h @@ +672,5 @@ > } > > + bool isUnparenthesizedNameForRHSAnonFunction(ParseNode* pn) { > + return pn->isKind(ParseNodeKind::Name) && !pn->isInParens(); > + } If possible, please delete these two methods (checkAndSetIsDirectRHSAnonFunction and isUnparenthesizedNameForRHSAnonFunction) from the ParseHandler interface, and handle this case in FPH::newAssignment(), addPropertyDefinition(), and newExportDefaultDeclaration(). (FPH can have private methods for any shared code. I'm not worried about the extra branch per local variable declaration; if you are, add a method newAssignmentWithoutCheckingForSetFunctionName.) Rationale: make the Parser code read better; make it harder for BinAST to get this bit wrong; eliminate the rather subtle "this boolean doesn't matter because that other method is a no-op" comment in SyntaxParseHandler; concentrate responsibility for this bit's correctness in one class (FPH). The ParseHandler interface is already a bit of a dumping ground for complexity, and it's in constant danger of getting worse. To fight this, we should try to make it syntax-driven and applicative (in the functional-programming sense). In a perfect world, it would be nothing but `newFoo()` methods, one for each spec grammar production, with no setters at all. ::: js/src/frontend/NameFunctions.cpp @@ +540,5 @@ > case ParseNodeKind::Import: > case ParseNodeKind::ExportFrom: > case ParseNodeKind::ExportDefault: > MOZ_ASSERT(cur->isArity(PN_BINARY)); > + // The left halves of Import and ExportFrom don't contain any Heh. Now the comment makes the reader wonder, "But what about ExportDefault?" I guess I would change it back, but it's up to you.

(In reply to Jason Orendorff [:jorendorff] from comment #4) > ::: js/src/frontend/NameFunctions.cpp > @@ +540,5 @@ > > case ParseNodeKind::Import: > > case ParseNodeKind::ExportFrom: > > case ParseNodeKind::ExportDefault: > > MOZ_ASSERT(cur->isArity(PN_BINARY)); > > + // The left halves of Import and ExportFrom don't contain any > > Heh. Now the comment makes the reader wonder, "But what about ExportDefault?" > > I guess I would change it back, but it's up to you. ExportDefault can contain arbitrary expressions (for the "export default AssignExpression" form). That's why I wanted to exclude it from the "[...] don't contain any unconstrained expressions [...]" comment.

Comment on attachment 8960220 [details] [diff] [review] bug1371591-part3-store-atom.patch Review of attachment 8960220 [details] [diff] [review]: ----------------------------------------------------------------- Yes, I think this change is correct. Function names are a mess, before and after this patch. :-( It seems sloppy that we use explicitName() in so many case and displayAtom() in so many others, generally with no indication of whether it matters at all which one we're using. I wonder if we have bugs where displayAtom() is used on bound functions, producing different results depending on whether getUnresolvedName() has been called. Or other stuff like that. ::: js/src/vm/JSFunction.cpp @@ +1332,3 @@ > // Unnamed class expressions should not get a .name property at all. > if (name) > v.set(name); The `if (name)` test is unnecessary. If you agree, please remove it, and set v unconditionally.

(In reply to Jason Orendorff [:jorendorff] from comment #6) > Comment on attachment 8960220 [details] [diff] [review] > bug1371591-part3-store-atom.patch > > Review of attachment 8960220 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yes, I think this change is correct. > > Function names are a mess, before and after this patch. :-( It seems sloppy > that we use explicitName() in so many case and displayAtom() in so many > others, generally with no indication of whether it matters at all which one > we're using. I wonder if we have bugs where displayAtom() is used on bound > functions, producing different results depending on whether > getUnresolvedName() has been called. Or other stuff like that. Yeah, bug 1371593 and the new bug 1447362 both made displayAtom() for bound functions a bit unreliable. But even before that displayAtom() no longer gave consistent results, for example because of syntax parsing: --- function testDisplayName(directive) { eval(` function outer() { "${directive}"; function inner() { return function(){}; } print(displayName(inner())); } outer(); `); } // Prints "inner/<" with syntax parsing enabled. testDisplayName(); // Prints "outer/inner/<" with syntax parsing disabled. testDisplayName("use asm"); --- And nobody seems to care. :-) > ::: js/src/vm/JSFunction.cpp > @@ +1332,3 @@ > > // Unnamed class expressions should not get a .name property at all. > > if (name) > > v.set(name); > > The `if (name)` test is unnecessary. If you agree, please remove it, and set > v unconditionally. bug 1447362 removes this test.

(In reply to Jason Orendorff [:jorendorff] from comment #4) > If possible, please delete these two methods > (checkAndSetIsDirectRHSAnonFunction and > isUnparenthesizedNameForRHSAnonFunction) from the ParseHandler interface, > and handle this case in FPH::newAssignment(), addPropertyDefinition(), and > newExportDefaultDeclaration(). (FPH can have private methods for any shared > code. I'm not worried about the extra branch per local variable declaration; > if you are, add a method newAssignmentWithoutCheckingForSetFunctionName.) FPH::newAssignment() and FPH::newExportDefaultDeclaration() are easy to change, but FPH::addPropertyDefinition() is a tricky one, because we need to check for anonymous functions before calling FoldConstants [2], but also want to call FoldConstants before marking the object literal as non-constant [2] resp. calling addPropertyDefinition(). [1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/src/frontend/Parser.cpp#9536-9537 [2] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/src/frontend/Parser.cpp#9562-9563

Updated per review comments, carrying r+.

Update part 3 to apply on updated part 1.

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

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/068989e6ca33 Part 1: Move parenthesized identifier check to parser. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be9e57bbef Part 2: Remove extra calls in SetFunctionNameIfNoOwnName. r=jorendorff https://hg.mozilla.org/integration/mozilla-inbound/rev/312e1b24fca5 Part 3: Store dynamic function names into the name atom instead of a property. r=jorendorff

https://hg.mozilla.org/mozilla-central/rev/068989e6ca33 https://hg.mozilla.org/mozilla-central/rev/c5be9e57bbef https://hg.mozilla.org/mozilla-central/rev/312e1b24fca5