Bug 212016

Summary: [bmalloc] Fix OOM errors on MIPS after r261667
Product: WebKit Reporter: Paulo Matos <pmatos>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED FIXED    
Severity: Normal CC: keith_miller, mark.lam, rmorisset, saam, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Other   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Paulo Matos 2020-05-18 04:40:16 PDT 
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.
Comment 1 Caio Lima 2020-05-22 06:47:09 PDT 
Created attachment 400042 [details]
Patch
Comment 2 Yusuke Suzuki 2020-05-22 11:41:36 PDT 
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?
Comment 3 Caio Lima 2020-05-22 12:37:00 PDT 
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.
Comment 4 Caio Lima 2020-05-22 12:58:25 PDT 
Created attachment 400068 [details]
Patch
Comment 5 Yusuke Suzuki 2020-05-22 13:41:21 PDT 
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.
Comment 6 Caio Lima 2020-05-23 06:57:49 PDT 
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.
Comment 7 Caio Lima 2020-05-23 07:00:37 PDT 
Created attachment 400120 [details]
Patch
Comment 8 Caio Lima 2020-05-23 09:23:21 PDT 
Comment on attachment 400120 [details]
Patch

Thank you very much for the review
Comment 9 EWS 2020-05-23 09:35:33 PDT 
Committed r262100: <https://trac.webkit.org/changeset/262100>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400120 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-23 09:36:18 PDT 
<rdar://problem/63570045>