Skip to content

Commit

Permalink
Stop emitting AllocObjectLiteral for object literals (facebook#135)
Browse files Browse the repository at this point in the history 
Summary:
Original Author: neildhar@meta.com
Original Git: bbe7966

Now that the general case is fast, and generates much better code in
debug mode, stop emitting `AllocObjectLiteral` for object literals in
IRGen. This allows us to compile large JSON files with reasonable
performance and output in both release and debug builds.

Closes facebook#135

Original Reviewed By: tmikov

Original Revision: D47724425

Differential Revision: D50460149
  • Loading branch information
@neildhar @facebook-github-bot
neildhar authored and facebook-github-bot committed Oct 19, 2023
1 parent 7ec3603 commit 374d101
Show file tree
Hide file tree
Showing 8 changed files with 90 additions and 91 deletions.
39 changes: 0 additions & 39 deletions lib/IRGen/ESTreeIRGen-expr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1072,13 +1072,9 @@ Value *ESTreeIRGen::genObjectExpr(ESTree::ObjectExpressionNode *Expr) {
ESTree::PropertyNode *protoProperty = nullptr;

uint32_t numComputed = 0;
bool hasSpread = false;
bool hasAccessor = false;
bool hasDuplicateProperty = false;

for (auto &P : Expr->_properties) {
if (llvh::isa<ESTree::SpreadElementNode>(&P)) {
hasSpread = true;
continue;
}

Expand Down Expand Up @@ -1115,10 +1111,8 @@ Value *ESTreeIRGen::genObjectExpr(ESTree::ObjectExpressionNode *Expr) {
PropertyValue *propValue = &propMap[propName];
if (prop->_kind->str() == "get") {
propValue->setGetter(cast<ESTree::FunctionExpressionNode>(prop->_value));
hasAccessor = true;
} else if (prop->_kind->str() == "set") {
propValue->setSetter(cast<ESTree::FunctionExpressionNode>(prop->_value));
hasAccessor = true;
} else {
assert(prop->_kind->str() == "init" && "invalid PropertyNode kind");
// We record the propValue if this is a regular property
Expand All @@ -1128,7 +1122,6 @@ Value *ESTreeIRGen::genObjectExpr(ESTree::ObjectExpressionNode *Expr) {
std::string key = (prop->_kind->str() + propName).str();
auto iterAndSuccess = firstLocMap.try_emplace(key, prop->getSourceRange());
if (!iterAndSuccess.second) {
hasDuplicateProperty = true;
Builder.getModule()->getContext().getSourceErrorManager().warning(
prop->getSourceRange(),
Twine("the property \"") + propName +
Expand All @@ -1139,38 +1132,6 @@ Value *ESTreeIRGen::genObjectExpr(ESTree::ObjectExpressionNode *Expr) {
}
}

// Heuristically determine if we emit AllocObjectLiteral.
// We do so if there is no computed key, no __proto__, no spread element
// node, no duplicate properties, no accessors, and object literal is not
// empty.
if (numComputed == 0 && !protoProperty && !hasSpread &&
!hasDuplicateProperty && !hasAccessor && propMap.size()) {
AllocObjectLiteralInst::ObjectPropertyMap objPropMap;
// It is safe to assume that there is no computed keys, and
// no __proto__.
for (auto &P : Expr->_properties) {
auto *prop = cast<ESTree::PropertyNode>(&P);
assert(
!prop->_computed &&
"Cannot handle computed key in AllocObjectLiteral");

// We are reusing the storage, so make sure it is cleared at every
// iteration.
stringStorage.clear();

llvh::StringRef keyStr = propertyKeyAsString(stringStorage, prop->_key);
auto *Key = Builder.getLiteralString(keyStr);
assert(
propMap[keyStr].valueNode == prop->_value &&
"Should only have one value for each property.");
auto value =
genExpression(prop->_value, Builder.createIdentifier(keyStr));
objPropMap.push_back(std::pair<LiteralString *, Value *>(Key, value));
}

return Builder.createAllocObjectLiteralInst(objPropMap);
}

/// Attempt to determine whether we can directly use the value of the
/// __proto__ property as a parent when creating the object for an object
/// initializer, instead of setting it later with
Expand Down
4 changes: 2 additions & 2 deletions test/BCGen/SH/call-builtin-clobber-callee.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ function test_call_after_builtin() {
// CHECK-NEXT:%BB0:
// CHECK-NEXT: {loc0} %0 = HBCGetGlobalObjectInst (:object)
// CHECK-NEXT: {loc1} %1 = TryLoadGlobalPropertyInst (:any) {loc0} %0: object, "print": string
// CHECK-NEXT: {loc0} %2 = HBCAllocObjectFromBufferInst (:object) 1: number, "valueOf": string, null: null
// CHECK-NEXT: {loc0} %2 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: {loc2} %3 = HBCCreateEnvironmentInst (:environment)
// CHECK-NEXT: {loc2} %4 = HBCCreateFunctionInst (:object) %valueOf(): number, {loc2} %3: environment
// CHECK-NEXT: StorePropertyLooseInst {loc2} %4: object, {loc0} %2: object, "valueOf": string
// CHECK-NEXT: StoreNewOwnPropertyInst {loc2} %4: object, {loc0} %2: object, "valueOf": string, true: boolean
// CHECK-NEXT: {stack[0]} %6 = HBCLoadConstInst (:number) 3: number
// CHECK-NEXT: {stack[4]} %7 = ImplicitMovInst (:undefined) undefined: undefined
// CHECK-NEXT: {stack[3]} %8 = ImplicitMovInst (:empty) empty: empty
Expand Down
11 changes: 7 additions & 4 deletions test/IRGen/__proto__-shorthand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,14 @@ function protoShorthandMix2(func) {
// CHECK-NEXT: StoreFrameInst %0: any, [func]: any
// CHECK-NEXT: StoreFrameInst undefined: undefined, [__proto__]: any
// CHECK-NEXT: StoreFrameInst 42: number, [__proto__]: any
// CHECK-NEXT: %4 = LoadFrameInst (:any) [__proto__]: any
// CHECK-NEXT: %5 = AllocObjectLiteralInst (:object) "__proto__": string, %4: any, "a": string, 2: number, "b": string, 3: number
// CHECK-NEXT: ReturnInst %5: object
// CHECK-NEXT: %4 = AllocObjectInst (:object) 3: number, empty: any
// CHECK-NEXT: %5 = LoadFrameInst (:any) [__proto__]: any
// CHECK-NEXT: StoreNewOwnPropertyInst %5: any, %4: object, "__proto__": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 2: number, %4: object, "a": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 3: number, %4: object, "b": string, true: boolean
// CHECK-NEXT: ReturnInst %4: object
// CHECK-NEXT:%BB1:
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT:function_end

// CHECK:function protoShorthandDup(func: any): any
Expand Down
22 changes: 12 additions & 10 deletions test/IRGen/array_object.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,18 @@ function foo(param) {
// CHECK-NEXT: StoreFrameInst %0: any, [param]: any
// CHECK-NEXT: StoreFrameInst undefined: undefined, [obj]: any
// CHECK-NEXT: StoreFrameInst undefined: undefined, [foo]: any
// CHECK-NEXT: %4 = LoadFrameInst (:any) [param]: any
// CHECK-NEXT: %5 = AllocObjectLiteralInst (:object) "1": string, 2: number, "key": string, %4: any
// CHECK-NEXT: StoreFrameInst %5: object, [obj]: any
// CHECK-NEXT: %7 = AllocArrayInst (:object) 4: number, 1: number, 2: number, 3: number, 4: number
// CHECK-NEXT: StoreFrameInst %7: object, [foo]: any
// CHECK-NEXT: %9 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: %10 = LoadFrameInst (:any) [foo]: any
// CHECK-NEXT: StorePropertyLooseInst %10: any, %9: any, "field": string
// CHECK-NEXT: %4 = AllocObjectInst (:object) 2: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst 2: number, %4: object, "1": string, true: boolean
// CHECK-NEXT: %6 = LoadFrameInst (:any) [param]: any
// CHECK-NEXT: StoreNewOwnPropertyInst %6: any, %4: object, "key": string, true: boolean
// CHECK-NEXT: StoreFrameInst %4: object, [obj]: any
// CHECK-NEXT: %9 = AllocArrayInst (:object) 4: number, 1: number, 2: number, 3: number, 4: number
// CHECK-NEXT: StoreFrameInst %9: object, [foo]: any
// CHECK-NEXT: %11 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: %12 = LoadFrameInst (:any) [foo]: any
// CHECK-NEXT: %13 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: StorePropertyLooseInst %13: any, %12: any, 5: number
// CHECK-NEXT: StorePropertyLooseInst %12: any, %11: any, "field": string
// CHECK-NEXT: %14 = LoadFrameInst (:any) [foo]: any
// CHECK-NEXT: %15 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: StorePropertyLooseInst %15: any, %14: any, 5: number
// CHECK-NEXT: ReturnInst undefined: undefined
// CHECK-NEXT:function_end
19 changes: 10 additions & 9 deletions test/IRGen/es6/tagged-template.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,16 +171,17 @@ function helloWorld() {
// CHECK-NEXT:frame = [obj: any]
// CHECK-NEXT:%BB0:
// CHECK-NEXT: StoreFrameInst undefined: undefined, [obj]: any
// CHECK-NEXT: %1 = LoadPropertyInst (:any) globalObject: object, "dummy": string
// CHECK-NEXT: %2 = AllocObjectLiteralInst (:object) "func": string, %1: any
// CHECK-NEXT: StoreFrameInst %2: object, [obj]: any
// CHECK-NEXT: %4 = GetTemplateObjectInst (:any) 5: number, true: boolean, "hello world!": string
// CHECK-NEXT: %5 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: %6 = LoadPropertyInst (:any) %5: any, "func": string
// CHECK-NEXT: %7 = CallInst (:any) %6: any, empty: any, empty: any, undefined: undefined, %5: any, %4: any
// CHECK-NEXT: ReturnInst %7: any
// CHECK-NEXT: %1 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: %2 = LoadPropertyInst (:any) globalObject: object, "dummy": string
// CHECK-NEXT: StoreNewOwnPropertyInst %2: any, %1: object, "func": string, true: boolean
// CHECK-NEXT: StoreFrameInst %1: object, [obj]: any
// CHECK-NEXT: %5 = GetTemplateObjectInst (:any) 5: number, true: boolean, "hello world!": string
// CHECK-NEXT: %6 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: %7 = LoadPropertyInst (:any) %6: any, "func": string
// CHECK-NEXT: %8 = CallInst (:any) %7: any, empty: any, empty: any, undefined: undefined, %6: any, %5: any
// CHECK-NEXT: ReturnInst %8: any
// CHECK-NEXT:%BB1:
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT:function_end

// CHECK:function callExpr(): any
Expand Down
35 changes: 25 additions & 10 deletions test/IRGen/json.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,29 @@ var json = {
// CHECK-NEXT: DeclareGlobalVarInst "json": string
// CHECK-NEXT: %1 = AllocStackInst (:any) $?anon_0_ret: any
// CHECK-NEXT: StoreStackInst undefined: undefined, %1: any
// CHECK-NEXT: %3 = AllocArrayInst (:object) 2: number, "GML": string, "XML": string
// CHECK-NEXT: %4 = AllocObjectLiteralInst (:object) "para": string, "A meta-markup language, used to create markup languages such as DocBook.": string, "GlossSeeAlso": string, %3: object
// CHECK-NEXT: %5 = AllocObjectLiteralInst (:object) "ID": string, "SGML": string, "SortAs": string, "SGML": string, "GlossTerm": string, "Standard Generalized Markup Language": string, "Acronym": string, "SGML": string, "Abbrev": string, "ISO 8879:1986": string, "GlossDef": string, %4: object, "GlossSee": string, "markup": string
// CHECK-NEXT: %6 = AllocObjectLiteralInst (:object) "GlossEntry": string, %5: object
// CHECK-NEXT: %7 = AllocObjectLiteralInst (:object) "title": string, "S": string, "GlossList": string, %6: object
// CHECK-NEXT: %8 = AllocObjectLiteralInst (:object) "title": string, "example glossary": string, "GlossDiv": string, %7: object
// CHECK-NEXT: %9 = AllocObjectLiteralInst (:object) "glossary": string, %8: object
// CHECK-NEXT: StorePropertyLooseInst %9: object, globalObject: object, "json": string
// CHECK-NEXT: %11 = LoadStackInst (:any) %1: any
// CHECK-NEXT: ReturnInst %11: any
// CHECK-NEXT: %3 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: %4 = AllocObjectInst (:object) 2: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst "example glossary": string, %4: object, "title": string, true: boolean
// CHECK-NEXT: %6 = AllocObjectInst (:object) 2: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst "S": string, %6: object, "title": string, true: boolean
// CHECK-NEXT: %8 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: %9 = AllocObjectInst (:object) 7: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst "SGML": string, %9: object, "ID": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst "SGML": string, %9: object, "SortAs": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst "Standard Generalized Markup Language": string, %9: object, "GlossTerm": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst "SGML": string, %9: object, "Acronym": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst "ISO 8879:1986": string, %9: object, "Abbrev": string, true: boolean
// CHECK-NEXT: %15 = AllocObjectInst (:object) 2: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst "A meta-markup language, used to create markup languages such as DocBook.": string, %15: object, "para": string, true: boolean
// CHECK-NEXT: %17 = AllocArrayInst (:object) 2: number, "GML": string, "XML": string
// CHECK-NEXT: StoreNewOwnPropertyInst %17: object, %15: object, "GlossSeeAlso": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst %15: object, %9: object, "GlossDef": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst "markup": string, %9: object, "GlossSee": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst %9: object, %8: object, "GlossEntry": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst %8: object, %6: object, "GlossList": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst %6: object, %4: object, "GlossDiv": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst %4: object, %3: object, "glossary": string, true: boolean
// CHECK-NEXT: StorePropertyLooseInst %3: object, globalObject: object, "json": string
// CHECK-NEXT: %26 = LoadStackInst (:any) %1: any
// CHECK-NEXT: ReturnInst %26: any
// CHECK-NEXT:function_end
44 changes: 30 additions & 14 deletions test/IRGen/object_literals.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,10 @@ function accessorObjectLiteral2(func) {
// CHECK-NEXT:%BB0:
// CHECK-NEXT: %0 = LoadParamInst (:any) %func: any
// CHECK-NEXT: StoreFrameInst %0: any, [func]: any
// CHECK-NEXT: %2 = AllocObjectLiteralInst (:object) "prop1": string, 10: number
// CHECK-NEXT: %3 = AllocObjectLiteralInst (:object) "prop1": string, 10: number
// CHECK-NEXT: %2 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst 10: number, %2: object, "prop1": string, true: boolean
// CHECK-NEXT: %4 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst 10: number, %4: object, "prop1": string, true: boolean
// CHECK-NEXT: ReturnInst undefined: undefined
// CHECK-NEXT:function_end

Expand All @@ -122,22 +124,34 @@ function accessorObjectLiteral2(func) {
// CHECK-NEXT:%BB0:
// CHECK-NEXT: %0 = LoadParamInst (:any) %func: any
// CHECK-NEXT: StoreFrameInst %0: any, [func]: any
// CHECK-NEXT: %2 = AllocObjectLiteralInst (:object) "a": string, 1: number, "b": string, 2: number, "c": string, 3: number, "d": string, 4: number, "5": string, 5: number, "6": string, 6: number
// CHECK-NEXT: %2 = AllocObjectInst (:object) 6: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst 1: number, %2: object, "a": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 2: number, %2: object, "b": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 3: number, %2: object, "c": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 4: number, %2: object, "d": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 5: number, %2: object, "5": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 6: number, %2: object, "6": string, true: boolean
// CHECK-NEXT: ReturnInst %2: object
// CHECK-NEXT:%BB1:
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT:function_end

// CHECK:function nestedAllocObjectLiteral(func: any): any
// CHECK-NEXT:frame = [func: any]
// CHECK-NEXT:%BB0:
// CHECK-NEXT: %0 = LoadParamInst (:any) %func: any
// CHECK-NEXT: StoreFrameInst %0: any, [func]: any
// CHECK-NEXT: %2 = AllocObjectLiteralInst (:object) "1": string, 100: number, "2": string, 200: number
// CHECK-NEXT: %3 = AllocObjectLiteralInst (:object) "a": string, 10: number, "b": string, %2: object, "c": string, "hello": string, "d": string, null: null
// CHECK-NEXT: ReturnInst %3: object
// CHECK-NEXT: %2 = AllocObjectInst (:object) 4: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst 10: number, %2: object, "a": string, true: boolean
// CHECK-NEXT: %4 = AllocObjectInst (:object) 2: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst 100: number, %4: object, "1": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 200: number, %4: object, "2": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst %4: object, %2: object, "b": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst "hello": string, %2: object, "c": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst null: null, %2: object, "d": string, true: boolean
// CHECK-NEXT: ReturnInst %2: object
// CHECK-NEXT:%BB1:
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT:function_end

// CHECK:function duplicatedObjectLiteral(func: any): any
Expand Down Expand Up @@ -219,13 +233,15 @@ function accessorObjectLiteral2(func) {
// CHECK-NEXT: %0 = LoadParamInst (:any) %func: any
// CHECK-NEXT: StoreFrameInst %0: any, [func]: any
// CHECK-NEXT: StoreFrameInst undefined: undefined, [obj]: any
// CHECK-NEXT: %3 = AllocObjectLiteralInst (:object) "a": string, 10: number, "b": string, 20: number
// CHECK-NEXT: %3 = AllocObjectInst (:object) 2: number, empty: any
// CHECK-NEXT: StoreNewOwnPropertyInst 10: number, %3: object, "a": string, true: boolean
// CHECK-NEXT: StoreNewOwnPropertyInst 20: number, %3: object, "b": string, true: boolean
// CHECK-NEXT: StoreFrameInst %3: object, [obj]: any
// CHECK-NEXT: %5 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: %6 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: %7 = CallBuiltinInst (:any) [HermesBuiltin.copyDataProperties]: number, empty: any, empty: any, undefined: undefined, undefined: undefined, %5: object, %6: any
// CHECK-NEXT: StoreOwnPropertyInst 42: number, %5: object, "c": string, true: boolean
// CHECK-NEXT: ReturnInst %5: object
// CHECK-NEXT: %7 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: %8 = LoadFrameInst (:any) [obj]: any
// CHECK-NEXT: %9 = CallBuiltinInst (:any) [HermesBuiltin.copyDataProperties]: number, empty: any, empty: any, undefined: undefined, undefined: undefined, %7: object, %8: any
// CHECK-NEXT: StoreOwnPropertyInst 42: number, %7: object, "c": string, true: boolean
// CHECK-NEXT: ReturnInst %7: object
// CHECK-NEXT:%BB1:
// CHECK-NEXT: UnreachableInst
// CHECK-NEXT:function_end
Expand Down
7 changes: 4 additions & 3 deletions test/Optimizer/type_infer_fun_returns.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@ function g14(z) {
// CHECK-NEXT: %8 = CallInst (:any) %7: object, empty: any, empty: any, undefined: undefined, undefined: undefined
// CHECK-NEXT: %9 = BinaryAddInst (:string|number) %8: any, 1: number
// CHECK-NEXT: %10 = CallInst (:any) %6: any, empty: any, empty: any, undefined: undefined, undefined: undefined, %9: string|number
// CHECK-NEXT: %11 = CreateFunctionInst (:object) %m(): undefined
// CHECK-NEXT: %12 = AllocObjectLiteralInst (:object) "m": string, %11: object
// CHECK-NEXT: ReturnInst %12: object
// CHECK-NEXT: %11 = AllocObjectInst (:object) 1: number, empty: any
// CHECK-NEXT: %12 = CreateFunctionInst (:object) %m(): undefined
// CHECK-NEXT: StoreNewOwnPropertyInst %12: object, %11: object, "m": string, true: boolean
// CHECK-NEXT: ReturnInst %11: object
// CHECK-NEXT:%BB2:
// CHECK-NEXT: ReturnInst undefined: undefined
// CHECK-NEXT:function_end
Expand Down

0 comments on commit 374d101

Please sign in to comment.