Skip to content

Commit a4df935

Browse files
sosukesuzukisosukesuzuki
committed
[JSC] Array#join on Int32 arrays should use JSOnlyStringsAndInt32sJoiner for any separator
https://bugs.webkit.org/show_bug.cgi?id=321228 Reviewed by Yusuke Suzuki. 295918@main taught JSOnlyStringsAndInt32sJoiner to write Int32 elements directly into the result buffer, but fastArrayJoin's ALL_INT32_INDEXING_TYPES case was not updated and still went through the generic JSStringJoiner, allocating a numeric string per element. Only the empty-separator case was fast because arrayProtoFuncJoin has its own early path, so `ids.join("")` was ~2x faster than `ids.join(",")` / `ids.toString()`. This patch makes the ALL_INT32 case try JSOnlyStringsAndInt32sJoiner first, mirroring the ALL_CONTIGUOUS case, and falls back to the generic loop only when the array has holes. tryJoin is now templatized on the indexing shape so that the Int32Shape instantiation only checks for holes and skips the per-element isString() checks that the ContiguousShape one needs. baseline patched array-join-int32-separator 22.1538+-0.6142 ^ 10.9635+-0.7135 ^ definitely 2.0207x faster Tests: JSTests/microbenchmarks/array-join-int32-separator.js JSTests/stress/array-join-int32-separator.js * JSTests/microbenchmarks/array-join-int32-separator.js: Added. (next): (test): * JSTests/stress/array-join-int32-separator.js: Added. (shouldBe): (test): * Source/JavaScriptCore/runtime/ArrayPrototypeInlines.h: (JSC::fastArrayJoin): Canonical link: https://commits.webkit.org/319022@main
1 parent 8d33a8f commit a4df935

7 files changed

Lines changed: 117 additions & 55 deletions

File tree

‎JSTests/microbenchmarks/array-join-int32-separator.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
var seed = 42;
2+
function next() {
3+
seed = (seed + 0x9e3779b9) | 0;
4+
var t = seed ^ (seed >>> 16);
5+
t = Math.imul(t, 0x21f0aaad);
6+
t = t ^ (t >>> 15);
7+
t = Math.imul(t, 0x735a2d97);
8+
return (t ^ (t >>> 15)) & 0x7fffffff;
9+
}
10+
11+
var ids = [];
12+
for (var i = 0; i < 100; ++i)
13+
ids.push(next() % 1000000);
14+
15+
function test(array) {
16+
return array.join(",");
17+
}
18+
noInline(test);
19+
20+
var total = 0;
21+
for (var i = 0; i < 2e4; ++i)
22+
total += test(ids).length;
23+
if (total !== test(ids).length * 2e4)
24+
throw new Error("bad result: " + total);

‎JSTests/stress/array-join-int32-separator.js‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
function shouldBe(actual, expected) {
2+
if (actual !== expected)
3+
throw new Error('bad value: ' + actual);
4+
}
5+
6+
function test(array, separator) {
7+
return array.join(separator);
8+
}
9+
noInline(test);
10+
11+
for (var i = 0; i < testLoopCount; ++i) {
12+
shouldBe(test([], ","), ``);
13+
shouldBe(test([42], ","), `42`);
14+
shouldBe(test([-42], ", "), `-42`);
15+
shouldBe(test([1, 2, 3], ","), `1,2,3`);
16+
shouldBe(test([1, 2, 3], ", "), `1, 2, 3`);
17+
shouldBe(test([1, 2, 3], ""), `123`);
18+
shouldBe(test([1, 2, 3], undefined), `1,2,3`);
19+
shouldBe(test([1, 2, 3], "、"), `1、2、3`);
20+
shouldBe(test([0, -1, 2147483647, -2147483648], "&id="), `0&id=-1&id=2147483647&id=-2147483648`);
21+
shouldBe([10, 200, 3000].toString(), `10,200,3000`);
22+
23+
var array = new Array(3);
24+
array[0] = 1;
25+
array[2] = 3;
26+
shouldBe(test(array, "-"), `1--3`);
27+
shouldBe(test([1, , 3], ", "), `1, , 3`);
28+
}
29+
30+
Array.prototype[1] = "x";
31+
shouldBe(test([1, , 3], "-"), `1-x-3`);
32+
var array = new Array(3);
33+
array[0] = 1;
34+
array[2] = 3;
35+
shouldBe(test(array, ", "), `1, x, 3`);

‎Source/JavaScriptCore/dfg/DFGOperations.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1738,7 +1738,7 @@ static ALWAYS_INLINE JSString* arrayJoinWithStringSeparator(JSGlobalObject* glob
17381738
if (!separator->length() && (array->indexingType() == ArrayWithContiguous || array->indexingType() == ArrayWithInt32)) {
17391739
auto* butterfly = array->butterfly();
17401740
JSOnlyStringsAndInt32sJoiner joiner(StringView { });
1741-
auto* joined = joiner.tryJoin(globalObject, butterfly->contiguous().data(), length);
1741+
auto* joined = joiner.tryJoin<ContiguousShape>(globalObject, butterfly->contiguous().data(), length);
17421742
RETURN_IF_EXCEPTION(scope, { });
17431743
if (joined)
17441744
return joined;

‎Source/JavaScriptCore/runtime/ArrayPrototype.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,7 @@ JSC_DEFINE_HOST_FUNCTION(arrayProtoFuncJoin, (JSGlobalObject* globalObject, Call
444444
auto* butterfly = array->butterfly();
445445
unsigned length = butterfly->publicLength();
446446
JSOnlyStringsAndInt32sJoiner joiner(StringView { });
447-
auto* joined = joiner.tryJoin(globalObject, butterfly->contiguous().data(), length);
447+
auto* joined = joiner.tryJoin<ContiguousShape>(globalObject, butterfly->contiguous().data(), length);
448448
RETURN_IF_EXCEPTION(scope, { });
449449
if (joined)
450450
return JSValue::encode(joined);

‎Source/JavaScriptCore/runtime/ArrayPrototypeInlines.h‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -310,9 +310,15 @@ ALWAYS_INLINE JSString* fastArrayJoin(JSGlobalObject* globalObject, JSObject* th
310310
auto& butterfly = *thisObject->butterfly();
311311
if (length > butterfly.publicLength()) [[unlikely]]
312312
break;
313+
auto data = butterfly.contiguous().data();
314+
315+
JSOnlyStringsAndInt32sJoiner onlyInt32sJoiner(separator);
316+
if (auto joined = onlyInt32sJoiner.tryJoin<Int32Shape>(globalObject, data, length))
317+
RELEASE_AND_RETURN(scope, joined);
318+
RETURN_IF_EXCEPTION(scope, { });
319+
313320
joiner.reserveCapacity(globalObject, length);
314321
RETURN_IF_EXCEPTION(scope, { });
315-
auto data = butterfly.contiguous().data();
316322
bool holesKnownToBeOK = false;
317323
for (; i < length; ++i) {
318324
JSValue value = data[i].get();
@@ -339,7 +345,7 @@ ALWAYS_INLINE JSString* fastArrayJoin(JSGlobalObject* globalObject, JSObject* th
339345
bool holesKnownToBeOK = false;
340346

341347
JSOnlyStringsAndInt32sJoiner onlyStringsJoiner(separator);
342-
if (auto joined = onlyStringsJoiner.tryJoin(globalObject, data, length))
348+
if (auto joined = onlyStringsJoiner.tryJoin<ContiguousShape>(globalObject, data, length))
343349
RELEASE_AND_RETURN(scope, joined);
344350
RETURN_IF_EXCEPTION(scope, { });
345351

‎Source/JavaScriptCore/runtime/JSStringJoiner.cpp‎

Lines changed: 24 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ static inline String joinStrings(const JSStringJoiner::Entries& strings, std::sp
180180
return result;
181181
}
182182

183-
template<typename OutputCharacterType, typename SeparatorCharacterType>
183+
template<IndexingType indexingShape, typename OutputCharacterType, typename SeparatorCharacterType>
184184
static inline String joinStrings(JSGlobalObject* globalObject, const WriteBarrier<Unknown>* strings, unsigned size, std::span<const SeparatorCharacterType> separator, unsigned joinedLength)
185185
{
186186
VM& vm = globalObject->vm();
@@ -196,43 +196,31 @@ static inline String joinStrings(JSGlobalObject* globalObject, const WriteBarrie
196196
return { };
197197
}
198198

199-
auto appendString = [&](JSString* string) {
200-
unsigned length = string->length();
201-
string->resolveToBuffer(data.first(length));
202-
data = data.subspan(length);
199+
auto appendValue = [&](JSValue value) {
200+
if constexpr (indexingShape == ContiguousShape) {
201+
if (value.isString()) {
202+
JSString* string = asString(value);
203+
unsigned length = string->length();
204+
string->resolveToBuffer(data.first(length));
205+
data = data.subspan(length);
206+
return;
207+
}
208+
}
209+
ASSERT(value.isInt32());
210+
appendStringToData(data, value.asInt32());
203211
};
204212

205213
switch (separator.size()) {
206214
case 0: {
207-
for (unsigned i = 0; i < size; ++i) {
208-
JSValue value = strings[i].get();
209-
if (value.isString())
210-
appendString(asString(value));
211-
else {
212-
ASSERT(value.isInt32());
213-
appendStringToData(data, value.asInt32());
214-
}
215-
}
215+
for (unsigned i = 0; i < size; ++i)
216+
appendValue(strings[i].get());
216217
break;
217218
}
218219
default: {
219-
JSValue value = strings[0].get();
220-
if (value.isString())
221-
appendString(asString(value));
222-
else {
223-
ASSERT(value.isInt32());
224-
appendStringToData(data, value.asInt32());
225-
}
226-
220+
appendValue(strings[0].get());
227221
for (unsigned i = 1; i < size; ++i) {
228-
JSValue value = strings[i].get();
229222
appendStringToData(data, separator);
230-
if (value.isString())
231-
appendString(asString(value));
232-
else {
233-
ASSERT(value.isInt32());
234-
appendStringToData(data, value.asInt32());
235-
}
223+
appendValue(strings[i].get());
236224
}
237225
break;
238226
}
@@ -295,6 +283,7 @@ JSString* JSStringJoiner::joinImpl(JSGlobalObject* globalObject)
295283
return jsString(vm, WTF::move(result));
296284
}
297285

286+
template<IndexingType indexingShape>
298287
JSString* JSOnlyStringsAndInt32sJoiner::joinImpl(JSGlobalObject* globalObject, const WriteBarrier<Unknown>* data, unsigned length)
299288
{
300289
VM& vm = globalObject->vm();
@@ -313,19 +302,22 @@ JSString* JSOnlyStringsAndInt32sJoiner::joinImpl(JSGlobalObject* globalObject, c
313302

314303
String result;
315304
if (m_isAll8Bit)
316-
result = joinStrings<Latin1Character>(globalObject, data, length, m_separator.span8(), totalLength);
305+
result = joinStrings<indexingShape, Latin1Character>(globalObject, data, length, m_separator.span8(), totalLength);
317306
else {
318307
if (m_separator.is8Bit())
319-
result = joinStrings<char16_t>(globalObject, data, length, m_separator.span8(), totalLength);
308+
result = joinStrings<indexingShape, char16_t>(globalObject, data, length, m_separator.span8(), totalLength);
320309
else
321-
result = joinStrings<char16_t>(globalObject, data, length, m_separator.span16(), totalLength);
310+
result = joinStrings<indexingShape, char16_t>(globalObject, data, length, m_separator.span16(), totalLength);
322311
}
323312

324313
RETURN_IF_EXCEPTION(scope, { });
325314

326315
return jsString(vm, WTF::move(result));
327316
}
328317

318+
template JSString* JSOnlyStringsAndInt32sJoiner::joinImpl<Int32Shape>(JSGlobalObject*, const WriteBarrier<Unknown>*, unsigned);
319+
template JSString* JSOnlyStringsAndInt32sJoiner::joinImpl<ContiguousShape>(JSGlobalObject*, const WriteBarrier<Unknown>*, unsigned);
320+
329321
} // namespace JSC
330322

331323
WTF_ALLOW_UNSAFE_BUFFER_USAGE_END

‎Source/JavaScriptCore/runtime/JSStringJoiner.h‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -234,45 +234,50 @@ class JSOnlyStringsAndInt32sJoiner {
234234
{
235235
}
236236

237+
template<IndexingType indexingShape>
237238
JSString* tryJoin(JSGlobalObject* globalObject, const WriteBarrier<Unknown>* data, unsigned length)
238239
{
240+
static_assert(indexingShape == Int32Shape || indexingShape == ContiguousShape);
241+
239242
if (length == 1) {
240243
JSValue value = data[0].get();
241244
if (!value)
242245
return nullptr;
243-
if (value.isString())
244-
return asString(value);
245-
if (value.isInt32()) {
246-
m_accumulatedStringsLength = WTF::StringTypeAdapter<int32_t> { value.asInt32() }.length();
247-
return joinImpl(globalObject, data, length);
246+
if constexpr (indexingShape == ContiguousShape) {
247+
if (value.isString())
248+
return asString(value);
249+
if (!value.isInt32())
250+
return nullptr;
248251
}
249-
return nullptr;
252+
ASSERT(value.isInt32());
253+
m_accumulatedStringsLength = WTF::StringTypeAdapter<int32_t> { value.asInt32() }.length();
254+
return joinImpl<indexingShape>(globalObject, data, length);
250255
}
251256

252257
for (size_t i = 0; i < length; ++i) {
253258
JSValue value = data[i].get();
254259
if (!value)
255260
return nullptr;
256261

257-
if (value.isString()) {
258-
JSString* string = asString(value);
259-
m_accumulatedStringsLength += string->length();
260-
m_isAll8Bit &= string->is8Bit();
261-
continue;
262-
}
263-
264-
if (value.isInt32()) {
265-
m_accumulatedStringsLength += WTF::StringTypeAdapter<int32_t> { value.asInt32() }.length();
266-
continue;
262+
if constexpr (indexingShape == ContiguousShape) {
263+
if (value.isString()) {
264+
JSString* string = asString(value);
265+
m_accumulatedStringsLength += string->length();
266+
m_isAll8Bit &= string->is8Bit();
267+
continue;
268+
}
269+
if (!value.isInt32())
270+
return nullptr;
267271
}
268-
269-
return nullptr;
272+
ASSERT(value.isInt32());
273+
m_accumulatedStringsLength += WTF::StringTypeAdapter<int32_t> { value.asInt32() }.length();
270274
}
271275

272-
return joinImpl(globalObject, data, length);
276+
return joinImpl<indexingShape>(globalObject, data, length);
273277
}
274278

275279
private:
280+
template<IndexingType indexingShape>
276281
JSString* joinImpl(JSGlobalObject*, const WriteBarrier<Unknown>*, unsigned);
277282

278283
StringView m_separator;

0 commit comments

Comments
 (0)