Skip to content
Permalink
Browse files
[JSC] DFG AI should attempt to remove @tryGetById for String.replace
https://bugs.webkit.org/show_bug.cgi?id=245418

Reviewed by Ross Kirsling.

@tryGetById is adhoc one: it is used only in builtin code and used in very limited places.
So this patch introduces adhoc folding rules for @tryGetById to remove TryGetById attached before
String.replace. Unfortunately these @tryGetById are not usually removed since it is adhocly attached
in DFG fixup phase.
In this patch,

1. Ensure RegExp.prototype's properties via watchpoints.
2. Check structure in AI, and ensure that this structure is one for RegExpObject, its [[Prototype]] is RegExp.prototype, it is cacheable,
   and it does not have properties for particular names.

Then we fold TryGetById to RegExp's specific properties values. It eliminates these TryGetById and improves String.replace in DFG / FTL
by 65% (e.g. string-replace).

                                                         ToT                     Patched

    string-replace-generic                         33.0472+-0.1132     ^     31.9405+-0.1483        ^ definitely 1.0346x faster
    string-replace                                  3.3774+-0.0951     ^      2.0387+-0.0476        ^ definitely 1.6566x faster
    put-by-val-with-string-replace-and-transition
                                                    4.4385+-0.0233            4.4319+-0.0188
    string-replace-benchmark                       83.8759+-0.1186     ^     83.4655+-0.2393        ^ definitely 1.0049x faster
    string-replace-string                         380.3462+-0.7600          379.2646+-1.0203
    string-replace-empty                            3.2455+-0.0406     ^      2.0051+-0.1350        ^ definitely 1.6186x faster

* Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):

Canonical link: https://commits.webkit.org/254717@main
  • Loading branch information
Constellation committed Sep 21, 2022
1 parent ae9067a commit e289c324e666c229a72012180c87f9a3829f9982
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 1 deletion.
@@ -0,0 +1,15 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error('bad value: ' + actual);
}

function test(target)
{
var regexp = /OK/g;
regexp[Symbol.replace] = function() { return "Hello"; };
return "OKOK".replace(regexp, target);
}
noInline(test);

for (var i = 0; i < 1e5; ++i)
shouldBe(test("Replaced"), `Hello`);
@@ -0,0 +1,15 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error('bad value: ' + actual);
}

function test(target)
{
return "OKOK".replace(/OK/g, target);
}
noInline(test);

for (var i = 0; i < 1e5; ++i)
shouldBe(test("Replaced"), `ReplacedReplaced`);
RegExp.prototype[Symbol.replace] = function() { return "Hello"; }
shouldBe(test("Replaced"), `Hello`);
@@ -49,6 +49,7 @@
#include "NumberConstructor.h"
#include "PutByStatus.h"
#include "RegExpObject.h"
#include "RegExpPrototype.h"
#include "SetPrivateBrandStatus.h"
#include "StringObject.h"
#include "StructureCache.h"
@@ -3424,12 +3425,56 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
makeHeapTopForNode(node);
break;

case TryGetById:
case TryGetById: {
// This is very adhoc, but @tryGetById is not used in user code, and it is used adhocly in very limited places.
// So adhoc one is fine.
AbstractValue& value = forNode(node->child1());
if (value.m_structure.isFinite()
&& (node->child1().useKind() == CellUse || !(value.m_type & ~SpecCell))) {
if (RegisteredStructure structure = value.m_structure.onlyStructure()) {
JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
if (structure->typeInfo().type() == RegExpObjectType
&& !structure->hasPolyProto()
&& structure->storedPrototype() == globalObject->regExpPrototype()
&& !structure->isDictionary()
&& structure->propertyAccessesAreCacheable()
&& structure->propertyAccessesAreCacheableForAbsence()
&& m_graph.isWatchingRegExpPrimordialPropertiesWatchpoint(node)) {
UniquedStringImpl* uid = node->cacheableIdentifier().uid();

auto attemptToFold = [&](UniquedStringImpl* name, JSValue constant) -> bool {
if (uid != name)
return false;
unsigned attributes;
PropertyOffset offset = structure->getConcurrently(uid, attributes);
if (isValidOffset(offset))
return false;
didFoldClobberWorld();
setConstant(node, *m_graph.freeze(constant));
return true;
};

if (attemptToFold(m_vm.propertyNames->exec.impl(), globalObject->regExpProtoExecFunction()))
break;

if (attemptToFold(m_vm.propertyNames->global.impl(), globalObject->regExpProtoGlobalGetter()))
break;

if (attemptToFold(m_vm.propertyNames->unicode.impl(), globalObject->regExpProtoUnicodeGetter()))
break;

if (attemptToFold(m_vm.propertyNames->replaceSymbol.impl(), globalObject->regExpProtoSymbolReplaceFunction()))
break;
}
}
}

// FIXME: This should constant fold at least as well as the normal GetById case.
// https://bugs.webkit.org/show_bug.cgi?id=156422
clobberWorld();
makeHeapTopForNode(node);
break;
}

case GetPrivateNameById:
case GetByIdDirect:

0 comments on commit e289c32

Please sign in to comment.