Bug 230245 - [bmalloc] freeableMemory and footprint of Heap are completely broken
Summary: [bmalloc] freeableMemory and footprint of Heap are completely broken
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-13 21:39 PDT by Basuke Suzuki
Modified: 2021-10-01 11:47 PDT (History)
10 users (show)

See Also:


Attachments
PATCH (9.62 KB, patch)
2021-09-14 13:15 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
patch again. (9.67 KB, patch)
2021-09-15 20:05 PDT, Basuke Suzuki
ggaren: review+
Details | Formatted Diff | Diff
PATCH (11.30 KB, patch)
2021-09-21 16:02 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
PATCH (11.50 KB, patch)
2021-09-30 12:33 PDT, Basuke Suzuki
Basuke.Suzuki: review?
Basuke.Suzuki: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2021-09-13 21:39:22 PDT
They became huge numbers which means the overflow happens at some point.
Comment 2 Basuke Suzuki 2021-09-14 12:11:20 PDT
(In reply to Basuke Suzuki from comment #1)
> git bisect says the break happens on this commit:
> https://github.com/WebKit/WebKit/commit/
> 8e1ec1def4ca1b38f2fad2679d120adc9531ce08#diff-
> 34efcb839f5ce590e8868d478f3801379e2c680f863116a740b63b99598691d6

But this change is correct because newly allocated range by VM has entire physical page. So the bug exists before this commit and this commit made that bug visible.
Comment 3 Basuke Suzuki 2021-09-14 13:15:37 PDT
Created attachment 438160 [details]
PATCH
Comment 4 Basuke Suzuki 2021-09-14 14:02:35 PDT
Comment on attachment 438160 [details]
PATCH

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

> Source/bmalloc/bmalloc/Heap.cpp:585
> +        adjustFootprint(lock, range.totalPhysicalSize(), "allocateLarge");

These two lines are the fix.
Comment 5 Basuke Suzuki 2021-09-14 14:38:22 PDT
Comment on attachment 438160 [details]
PATCH

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

> Source/bmalloc/bmalloc/Heap.cpp:112
> +    constexpr bool verbose = false;

This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9
Comment 6 Geoffrey Garen 2021-09-15 12:52:31 PDT
Comment on attachment 438160 [details]
PATCH

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

> Source/bmalloc/bmalloc/Heap.cpp:100
> +    BUNUSED(label);
> +    BUNUSED(note);

label and note are used, so these statements appear to be unnecessary / incorrect.

> Source/bmalloc/bmalloc/Heap.cpp:103
> +    BASSERT(label || (amount >= 0 && result >= value) || (amount < 0 && result < value));

Why does having a label opt you out of this assertion?

Side note: Are you trying to do checked arithmetic here? If so, I'd prefer using something from CheckedArithmetic.h.

>> Source/bmalloc/bmalloc/Heap.cpp:112
>> +    constexpr bool verbose = false;
> 
> This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9

Does putting verbose inside Heap::adjustStat() not achieve the same goal, even though it is inlined?

> Source/bmalloc/bmalloc/Heap.cpp:113
> +    BUNUSED(note);

Do we need this? 'note' appears in this function.

> Source/bmalloc/bmalloc/Heap.cpp:126
> +    BUNUSED(note);

Ditto
Comment 7 Basuke Suzuki 2021-09-15 13:28:42 PDT
Comment on attachment 438160 [details]
PATCH

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

>> Source/bmalloc/bmalloc/Heap.cpp:100
>> +    BUNUSED(note);
> 
> label and note are used, so these statements appear to be unnecessary / incorrect.

Right. This was leftover from pre-cleanup code.

>> Source/bmalloc/bmalloc/Heap.cpp:103
>> +    BASSERT(label || (amount >= 0 && result >= value) || (amount < 0 && result < value));
> 
> Why does having a label opt you out of this assertion?
> 
> Side note: Are you trying to do checked arithmetic here? If so, I'd prefer using something from CheckedArithmetic.h.

For debugging purpose. I wanted to see the weird overflow using verbose output. On that case this assertion was not required. But that can be controlled by release/debug. I can remove this.

>>> Source/bmalloc/bmalloc/Heap.cpp:112
>>> +    constexpr bool verbose = false;
>> 
>> This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9
> 
> Does putting verbose inside Heap::adjustStat() not achieve the same goal, even though it is inlined?

This can control activating only footprint verbose output, for instance. If the result after optimization differ for moving this into adjustStat() then I will consider for moving, but they are practically same. Or I can extract the logging part from adjustStat and call separately for clarification.

>> Source/bmalloc/bmalloc/Heap.cpp:113
>> +    BUNUSED(note);
> 
> Do we need this? 'note' appears in this function.

Leftover. will remove.

>> Source/bmalloc/bmalloc/Heap.cpp:126
>> +    BUNUSED(note);
> 
> Ditto

Ditto. Thanks
Comment 8 Basuke Suzuki 2021-09-15 20:05:46 PDT
Created attachment 438316 [details]
patch again.

Porting CheckedArithmetic from WTF is too big to include this patch. That should be separated bug to be achieved.
Comment 9 Radar WebKit Bug Importer 2021-09-20 21:40:16 PDT
<rdar://problem/83339024>
Comment 10 Geoffrey Garen 2021-09-21 13:14:22 PDT
> Porting CheckedArithmetic from WTF is too big to include this patch. That
> should be separated bug to be achieved.

Fair point. I had forgotten that CheckedArithmetic was in a separate library.
Comment 11 Geoffrey Garen 2021-09-21 13:21:37 PDT
Comment on attachment 438316 [details]
patch again.

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

r=me

> Source/bmalloc/ChangeLog:3
> +        [bmalloc] freeableMemory and footprint of Heap are broken

Can you make this title more precise? Are freeableMemory and footprint literally completely broken in all cases all the time, or only in certain cases, or only broken in a particular way?

Can you say anything about how observable behavior changes after this change is made?
Comment 12 Basuke Suzuki 2021-09-21 16:02:11 PDT
Created attachment 438876 [details]
PATCH

Added example of output to display error information
Comment 13 Basuke Suzuki 2021-09-21 16:03:35 PDT
> r=me

Thank you very much.


> > Source/bmalloc/ChangeLog:3
> > +        [bmalloc] freeableMemory and footprint of Heap are broken
> 
> Can you make this title more precise? Are freeableMemory and footprint
> literally completely broken in all cases all the time, or only in certain
> cases, or only broken in a particular way?
> 
> Can you say anything about how observable behavior changes after this change
> is made?


I've added those description in ChangeLog,
Comment 14 EWS 2021-09-21 17:00:14 PDT
Committed r282850 (241981@main): <https://commits.webkit.org/241981@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438876 [details].
Comment 15 Alexey Proskuryakov 2021-09-29 14:32:30 PDT
We started seeing many unreproducible crashes in bmalloc::Heap::decommitLargeRange on the bots recently, such as below. May need to revert this change to see if that fixes those.

https://bug-230719-attachments.webkit.org/attachment.cgi?id=439093
https://bug-230866-attachments.webkit.org/attachment.cgi?id=439406
Comment 16 Eric Hutchison 2021-09-29 14:45:15 PDT
Reverted r282850 for reason:

Patch causing many crashes in bmalloc::Heap::decommitLargeRange

Committed r283264 (242293@main): <https://commits.webkit.org/242293@main>
Comment 17 Basuke Suzuki 2021-09-29 15:59:15 PDT
(In reply to Alexey Proskuryakov from comment #15)
> We started seeing many unreproducible crashes in
> bmalloc::Heap::decommitLargeRange on the bots recently, such as below. May
> need to revert this change to see if that fixes those.
> 
> https://bug-230719-attachments.webkit.org/attachment.cgi?id=439093
> https://bug-230866-attachments.webkit.org/attachment.cgi?id=439406

Sure. Please do it. But I don't have idea why this causes the crash.
Comment 18 Basuke Suzuki 2021-09-29 16:03:33 PDT
Wait a minute, are those debug build? If so, then that might be because I've added assertions there.
Comment 19 Alexey Proskuryakov 2021-09-29 17:05:46 PDT
Yes, these look like assertions (at least two separate ones, judging from crash logs).
Comment 20 Basuke Suzuki 2021-09-29 17:33:14 PDT
(In reply to Alexey Proskuryakov from comment #19)
> Yes, these look like assertions (at least two separate ones, judging from
> crash logs).

Give me an advice. Here's the background.
1. The numbers are broken before this patch landed
2. Those numbers are debugging information which is not used at runtime (as far as I know)
3. I fixed the one I've found and it seems the number is now healthy.
4. I've added assertion to detect future regression
5. Actually there's still case where the numbers are incorrect.
6. I basically need this fixed number to see my other patches effect.

I think to remove assertion is the best for now. What do you think?
Comment 21 Basuke Suzuki 2021-09-30 12:33:20 PDT
Created attachment 439770 [details]
PATCH

Make assertion optional.
Comment 22 Basuke Suzuki 2021-09-30 12:34:39 PDT
Comment on attachment 439770 [details]
PATCH

The optional assertion in adjustStat is required to find the hidden issue.