Bug 65840 - Fix comment in NewSpace.h
Summary: Fix comment in NewSpace.h
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 65458
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-07 23:17 PDT by Zoltan Herczeg
Modified: 2011-08-10 23:45 PDT (History)
3 users (show)

See Also:


Attachments
patch (1.93 KB, patch)
2011-08-07 23:23 PDT, Zoltan Herczeg
ggaren: review-
zherczeg: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.