Bug 65840 – Fix comment in NewSpace.h

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Zoltan Herczeg]

Wrong comments can be misleading when you need to fix a bug.

[Zoltan Herczeg]

Created attachment 103203 [details]
patch

[Gavin Barraclough]

cc'ing GC experts

[Geoffrey Garen]

Comment on attachment 103203 [details]
patch

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

> Source/JavaScriptCore/heap/NewSpace.h:88
> +        // [ 128 - preciseStep, 256... 1024 )

I don't think this is true: the first imprecise size class is exactly 128, not 128 - preciseStep.

[Zoltan Herczeg]

> I don't think this is true: the first imprecise size class is exactly 128, not 128 - preciseStep.

See bug 65458 (Depends on). The bug was that an allocation of 103 bytes is bigger than 95, which is the end of last precise class on 64 bit. So it is allocated from the first inprecise class. Or should it go to a precise class?

[Geoffrey Garen]

> See bug 65458 (Depends on). The bug was that an allocation of 103 bytes is bigger than 95, which is the end of last precise class on 64 bit. So it is allocated from the first inprecise class. Or should it go to a precise class?

Where to allocate is one question. Independently, this comment describes the size of the cells in the class. That size is 128, not 128 - preciseStep.

[Zoltan Herczeg]

> Where to allocate is one question. Independently, this comment describes the size of the cells in the class. That size is 128, not 128 - preciseStep.

Are you sure? The comment said:

// [ 128, 256... 1024 )

The size of the first block is really 128, but the second is 128 as well, not 256, and the last is 128 as well, not 1024. Instead I think the comment describes the ranges, the first range is [128-256), the second is [256-384) ...

The growth rate is clearly linear:

static const size_t preciseCutoff = 128;
static const size_t impreciseStep = preciseCutoff;
return m_impreciseSizeClasses[(bytes - 1) / impreciseStep];

[Geoffrey Garen]

(gdb) disp cellSize
(gdb) run
[Switching to process 3491]
Running…
3: cellSize = 32
(gdb) c
Continuing.
3: cellSize = 64
(gdb) 
Continuing.
3: cellSize = 96
(gdb) 
Continuing.
4: cellSize = 128
(gdb) 
Continuing.
4: cellSize = 256
(gdb) 
Continuing.
4: cellSize = 384
(gdb) 
Continuing.
4: cellSize = 512
(gdb) 
Continuing.
4: cellSize = 640
(gdb) 
Continuing.
4: cellSize = 768
(gdb) 
Continuing.
4: cellSize = 896

[Zoltan Herczeg]

Oh, those brackets were confused me. [a,b) in mathematics means a range of numbers, where x >= a and x < b. Sorry for the misunderstanding.