Bug 212016 – [bmalloc] Fix OOM errors on MIPS after r261667

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Paulo Matos]

The following tests started to fail around r261680 and skipped on r261808.

The following JSC stress test failures have been introduced:
	jsc-layout-tests.yaml/js/script-tests/stack-overflow-regexp.js.layout
	jsc-layout-tests.yaml/js/script-tests/stack-overflow-regexp.js.layout-dfg-eager-no-cjit
	jsc-layout-tests.yaml/js/script-tests/stack-overflow-regexp.js.layout-no-cjit
	jsc-layout-tests.yaml/js/script-tests/stack-overflow-regexp.js.layout-no-llint
	stress/array-buffer-view-watchpoint-can-be-fired-in-really-add-in-dfg.js.default

These should not fail and might be related to recent changes regarding memory management.

[Caio Lima]

Created attachment 400042 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 400042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400042&action=review

> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:48
>          } else if (index < bits->begin()) {

Doesn't `if (bits == &sentinelBits) {` case require the underflow check too?

[Caio Lima]

Comment on attachment 400042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400042&action=review

>> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:48
>>          } else if (index < bits->begin()) {
> 
> Doesn't `if (bits == &sentinelBits) {` case require the underflow check too?

I missed it and I think it does. I'll add those.

[Caio Lima]

Created attachment 400068 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 400068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400068&action=review

r=me with comments.

> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:47
> +            newBegin = index < ObjectTypeTable::Bits::bitCountPerWord * 4 ? 0 : std::min<unsigned>(index, index - ObjectTypeTable::Bits::bitCountPerWord * 4);

Can you wrtie it like,

constexpr unsigned offsetForInitialAllocation = ObjectTypeTable::Bits::bitCountPerWord * 4;
if (index < offsetForInitialAllocation)
    newBegin = 0;
else
    newBegin = std::min<unsigned>(index, index - offsetForInitialAllocation);

> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:56
> +            newBegin =  bits->begin() < bits->count() ? 0 : std::min<unsigned>(index, bits->begin() - bits->count());

Ditto like this, since it is hard to read.

if (bits->begin() < bits->count())
    newBegin = 0;
else
    newBegin = std::min<unsigned>(index, bits->begin() - bits->count());

> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:66
> +            newEnd = std::numeric_limits<unsigned>::max() - bits->count() < bits->end() ? std::numeric_limits<unsigned>::max() : std::max<unsigned>(index + 1, bits->end() + bits->count());

Ditto.

[Caio Lima]

Comment on attachment 400068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400068&action=review

>> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:47
>> +            newBegin = index < ObjectTypeTable::Bits::bitCountPerWord * 4 ? 0 : std::min<unsigned>(index, index - ObjectTypeTable::Bits::bitCountPerWord * 4);
> 
> Can you wrtie it like,
> 
> constexpr unsigned offsetForInitialAllocation = ObjectTypeTable::Bits::bitCountPerWord * 4;
> if (index < offsetForInitialAllocation)
>     newBegin = 0;
> else
>     newBegin = std::min<unsigned>(index, index - offsetForInitialAllocation);

Changed. The only difference I applied was to use `newBegin = index - offsetForInitialAllocation;`, since it is not possible that `index` will ever be lower than `index - offsetForInitialAllocation` when `index >= offsetForInitialAllocation`.

>> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:56
>> +            newBegin =  bits->begin() < bits->count() ? 0 : std::min<unsigned>(index, bits->begin() - bits->count());
> 
> Ditto like this, since it is hard to read.
> 
> if (bits->begin() < bits->count())
>     newBegin = 0;
> else
>     newBegin = std::min<unsigned>(index, bits->begin() - bits->count());

Changed.

>> Source/bmalloc/bmalloc/ObjectTypeTable.cpp:66
>> +            newEnd = std::numeric_limits<unsigned>::max() - bits->count() < bits->end() ? std::numeric_limits<unsigned>::max() : std::max<unsigned>(index + 1, bits->end() + bits->count());
> 
> Ditto.

Changed.

[Caio Lima]

Created attachment 400120 [details]
Patch

[Caio Lima]

Comment on attachment 400120 [details]
Patch

Thank you very much for the review

[EWS]

Committed r262100: <https://trac.webkit.org/changeset/262100>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400120 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63570045>