Bug 65840

Summary: Fix comment in NewSpace.h
Product: WebKit Reporter: Zoltan Herczeg <zherczeg>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: barraclough, ggaren, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 65458    
Bug Blocks:    
Attachments:
Description Flags
patch ggaren: review-, ntim: commit-queue-

Description Zoltan Herczeg 2011-08-07 23:17:21 PDT
Wrong comments can be misleading when you need to fix a bug.
Comment 1 Zoltan Herczeg 2011-08-07 23:23:30 PDT
Created attachment 103203 [details]
patch
Comment 2 Gavin Barraclough 2011-08-08 12:08:24 PDT
cc'ing GC experts
Comment 3 Geoffrey Garen 2011-08-08 15:56:48 PDT
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.
Comment 4 Zoltan Herczeg 2011-08-09 02:36:31 PDT
> 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?
Comment 5 Geoffrey Garen 2011-08-09 11:49:45 PDT
> 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.
Comment 6 Zoltan Herczeg 2011-08-09 23:29:54 PDT
> 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];
Comment 7 Geoffrey Garen 2011-08-10 15:32:15 PDT
(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
Comment 8 Zoltan Herczeg 2011-08-10 23:45:07 PDT
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.