Bug 230283 – [bmalloc] Simplify LargeRange constructors and remove meaningless one

• Status: ASSIGNED
• Component: bmalloc

[Basuke Suzuki]

LargeRange is mainly created in Heap and most usages are simply memory region with full of physical pages. This patch add simple constructor with address and size, which generates LargeRange with full of pages.

Also the constructor which accept Range is used in only one place, shrinkLarge(), and that's also can be simplify with this new constructor because the range is only created to be passed to shrinkLarge(). Size information is fetched in shrinkLarge anyway and at worst, it is confusing not to use size information from the passed Range argument. Replacing the argument with simple void pointer and there's no change for its behavior  at all.

Actually I still cannot understand the behavior or meaning of physical page information stored in LargeRange. This is the first step of the investigation to simplify the case with full physical pages.

[Basuke Suzuki]

Created attachment 438187 [details]
PATCH

[Filip Pizlo]

Comment on attachment 438187 [details]
PATCH

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

I get that auto is fun and cool, and for some kinds of code, it may be the right thing to do.

But not in a malloc.  We spend lots more time reading malloc than writing it.  So, auto is a bad trade-off.  It makes the code less self-documenting and it takes away checking.  It makes the code less well documented because now I need to check that largeRanges is really a collection of LargeRanges to know that this is what you get from it and add to it, for example.  Using auto also takes away checking; I like that the code before your change is explicitly stating that it wants to work with LargeRanges, not just whatever types the collection has.

So, I think that most of your change just makes the code harder to read and less self-checked.  It's not a good trade-off for a malloc.

> Source/bmalloc/ChangeLog:19
> +        Actually I still cannot understand the behavior or meaning of physical page information stored in
> +        LargeRange. This is the first step of the investigation to simplify the case with full physical pages.

I strongly recommend you understand it deeply before making more changes.

> Source/bmalloc/bmalloc/Heap.cpp:68
> -        m_largeFree.add(LargeRange(base, Gigacage::size(gigacageKind(m_kind)), 0, 0, base));
> +        m_largeFree.add({ base, Gigacage::size(gigacageKind(m_kind)), 0, 0, base });

This change makes the code less clear.  I want to know that I'm creating LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:159
> -    for (LargeRange& range : m_largeFree) {
> +    for (auto& range : m_largeFree) {

This change makes the code less clear.  I want to know that range is a LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:247
> -    m_largeFree.add(LargeRange(chunk, size, startPhysicalSize, totalPhysicalSize, physicalEnd));
> +    m_largeFree.add({ chunk, size, startPhysicalSize, totalPhysicalSize, physicalEnd });

This change makes the code less clear.  I want to know that I'm creating LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:481
> -        std::pair<LargeRange, LargeRange> pair = range.split(prefixSize);
> +        auto pair = range.split(prefixSize);

This change makes the code less clear.  I want to know that it's a pair of LargeRanges.

> Source/bmalloc/bmalloc/Heap.cpp:487
> -        std::pair<LargeRange, LargeRange> pair = range.split(size);
> +        auto pair = range.split(size);

This change makes the code less clear.  I want to know that it's a pair of LargeRanges.

> Source/bmalloc/bmalloc/Heap.cpp:542
> -    LargeRange range = m_largeFree.remove(alignment, size);
> +    auto range = m_largeFree.remove(alignment, size);

This change makes the code less clear.  I want to know that it's a LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:574
> -        return LargeRange();
> +        return { };

This change makes the code less clear.  I want to know that I'm returning a LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:579
> -        return LargeRange();
> +        return { };

This change makes the code less clear.  I want to know that I'm returning a LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:584
> -        return LargeRange();
> +        return { };

This change makes the code less clear.  I want to know that I'm returning a LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:590
> -    return LargeRange(memory, size, size, size, static_cast<char*>(memory) + size);
> +    return { memory, size };

This change makes the code less clear.  I want to know that I'm returning a LargeRange.

> Source/bmalloc/bmalloc/Heap.cpp:612
> -    m_largeFree.add(LargeRange(object, size, size, size, static_cast<char*>(object) + size));
> +    m_largeFree.add({ object, size });

This change makes the code less clear.  I want to know that I'm creating a LargeRange.

[Basuke Suzuki]

Created attachment 438617 [details]
patch

Filip, thanks for reviewing.

(In reply to Filip Pizlo from comment #2) 
> But not in a malloc.  We spend lots more time reading malloc than writing
> it.

I agree with this, and I agree with this not only for malloc code, but for any kind of source code. `auto` helps to reduce the typing, but that's not the main purpose of auto. It helps to improve the readability of the code when it is used for clear deducible types by reducing the amount of volume required to read. That's my perspective.

From this, I agree with your review for some code, such as switching constructor to implicit deducing of class instantiation, but some improves the code quality with better readability. For instance,

1. auto in for-loop
The type of element is clear from the container and container type is as important as element type.

2. auto for complex type, i.e. std::pair<LargeFree, LargeFree>
Well, this is not so complex, but the variable name tells it is pair and the method name is split(). It is hard to think of other possibility, isn't it?

3. No number three.


But the focus of this patch is to simplify constructor to make the detail of physical memory management inside LargeFree.h. So I reverted my refactoring part of the changes from this patch. Please take a look again.

[Filip Pizlo]

(In reply to Basuke Suzuki from comment #3)
> Created attachment 438617 [details]
> patch
> 
> Filip, thanks for reviewing.
> 
> (In reply to Filip Pizlo from comment #2) 
> > But not in a malloc.  We spend lots more time reading malloc than writing
> > it.
> 
> I agree with this, and I agree with this not only for malloc code, but for
> any kind of source code.

I’m making the point that in malloc, the reading-time-to-writing-time ratio is greater than almost any other kind of code. 

> `auto` helps to reduce the typing, but that's not
> the main purpose of auto. It helps to improve the readability of the code
> when it is used for clear deducible types by reducing the amount of volume
> required to read. That's my perspective.
> 
> From this, I agree with your review for some code, such as switching
> constructor to implicit deducing of class instantiation, but some improves
> the code quality with better readability. For instance,
> 
> 1. auto in for-loop
> The type of element is clear from the container and container type is as
> important as element type.

It’s not clear enough if you use auto. Using auto means that if the type of the element changes then the code might still compile based on duck typing and that’s not good enough. Using auto means that anyone reading the code has to look at the collection type to work out what auto infers to, and that’s not good enough for me when reading my malloc code. 

> 
> 2. auto for complex type, i.e. std::pair<LargeFree, LargeFree>
> Well, this is not so complex, but the variable name tells it is pair and the
> method name is split(). It is hard to think of other possibility, isn't it?

I don’t like thinking about possibilities when reading malloc code. I prefer certainties. Malloc tends to have a shortage of certainties to begin with so I want the types to at least be something that is certain. 

> 
> 3. No number three.
> 
> 
> But the focus of this patch is to simplify constructor to make the detail of
> physical memory management inside LargeFree.h. So I reverted my refactoring
> part of the changes from this patch. Please take a look again.

Looking…

[Filip Pizlo]

Comment on attachment 438617 [details]
patch

I think this is fine - but if this refactoring is just so you can understand the code better then I have mixed feelings about it. 

From what I understand of what you’re trying to do, it seems you’d be better served by just hacking the scavenger to call munmap.

[Radar WebKit Bug Importer]

<rdar://problem/83374267>