Do not call memset() to initialize StubCache.

Do not call memset() to initialize StubCache.

Committed: https://code.google.com/p/v8/source/detail?r=10861

[ulan, 2012-02-27 12:24:20]

Please take a look.

Patch Set 1 contains the original patch from Iliyan.

Patch Set 2 is what I propose to take from the original patch.

Iliyan's message follows:

Most of the StubCache object is initialized with a single memset() to zero in
the constructor. This is wasteful of memory, as the memset will immediately
cause COW duplication of 4 pages (per v8 instance?), most of which may not
actually be used.

This patch removes the memsets() from the constructor, and constructs the object
on top of a calloc()-allocated buffer instread. Calloc() will most likely get
fresh pages via an anonymous mmap() behind the scenes, which are
already zeroed out by the kernel.

https://chromiumcodereview.appspot.com/9464054/diff/2001/src/stub-cache.cc
File src/stub-cache.cc (left):

https://chromiumcodereview.appspot.com/9464054/diff/2001/src/stub-cache.cc#ol...
src/stub-cache.cc:48: memset(primary_, 0, sizeof(primary_[0]) *
StubCache::kPrimaryTableSize);
It should be save to remove the memsets, because the isolate calls
StubCache::Initialize() shortly after constructing the stub cache.

[Sven Panne, 2012-02-27 13:03:29]

I intentionally commented on the first patch-set, because it contains both
affected files. My proposal is to delay the construction of the StubCache a bit,
and then do it in a single step via the constructor. This way we avoid having a
dangerous half-initialized object lying around, something which is always
worthwhile.

If it turns out that we need this 2-step construction, we can at least nuke the
boolean argument to Initialize.

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc#newcode1633
src/isolate.cc:1633: free(stub_cache_);
Leave this as-is.

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc#newcode1782
src/isolate.cc:1782: stub_cache_ = new (calloc(1, sizeof(StubCache)))
StubCache(this);
I think we can remove this line completely and replace it with something
below...

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc#newcode1839
src/isolate.cc:1839: stub_cache_->Initialize(create_heap_objects);
Remove this line.

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc#newcode1844
src/isolate.cc:1844: stub_cache_->Initialize(true);
I think we can move this Initialize past the condition and replace it with a
"stub_cache_ = new StubCache(this);" there, because I doubt that the
uninitialized StubCache can be used before this point, anyway.

http://codereview.chromium.org/9464054/diff/1/src/stub-cache.cc
File src/stub-cache.cc (left):

http://codereview.chromium.org/9464054/diff/1/src/stub-cache.cc#oldcode46
src/stub-cache.cc:46: StubCache::StubCache(Isolate* isolate) : isolate_(isolate)
{
Merge the constructor with the Initialize function below, assuming
create_heap_objects == true. This way, we only construct a nice, clean
initialized object.

[ulan, 2012-02-27 16:13:02]

Thanks for the comments, please take another look.

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc#newcode1782
src/isolate.cc:1782: stub_cache_ = new (calloc(1, sizeof(StubCache)))
StubCache(this);
On 2012/02/27 13:03:29, Sven Panne wrote:
> I think we can remove this line completely and replace it with something
> below...

Unfortunately, builtins.SetUp() assumes that stub_cache->isolate_ is
initialized.

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc#newcode1839
src/isolate.cc:1839: stub_cache_->Initialize(create_heap_objects);
On 2012/02/27 13:03:29, Sven Panne wrote:
> Remove this line.

Done.

http://codereview.chromium.org/9464054/diff/1/src/isolate.cc#newcode1844
src/isolate.cc:1844: stub_cache_->Initialize(true);
On 2012/02/27 13:03:29, Sven Panne wrote:
> I think we can move this Initialize past the condition and replace it with a
> "stub_cache_ = new StubCache(this);" there, because I doubt that the
> uninitialized StubCache can be used before this point, anyway.

Moved Initialize().

[Sven Panne, 2012-02-28 07:31:58]

lgtm