Last Comment Bug 720439 - GC: make jsgcchunk's backend allocator take arbitrary size and alignment
: GC: make jsgcchunk's backend allocator take arbitrary size and alignment
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Terrence Cole [:terrence]
:
Mentors:
: 696119 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-23 11:11 PST by Terrence Cole [:terrence]
Modified: 2012-04-12 12:17 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (35.67 KB, patch)
2012-01-26 15:33 PST, Terrence Cole [:terrence]
no flags Details | Diff | Review
v2: With review feedback (37.15 KB, patch)
2012-02-03 11:07 PST, Terrence Cole [:terrence]
igor: review-
Details | Diff | Review
v3: With second round of review feedback. (35.41 KB, patch)
2012-02-08 17:41 PST, Terrence Cole [:terrence]
igor: review+
Details | Diff | Review

Description Terrence Cole [:terrence] 2012-01-23 11:11:49 PST
Arbitrary up to a granularity of page size, but no further, for obvious reasons.

Both the new Nursery code and the new WriteBuffer code for GenerationalGC will require a different size and alignment from Chunks, but will need a very similar allocation mechanism.  We should just generalize the existing Chunk allocation code to handle the new users.
Comment 1 Terrence Cole [:terrence] 2012-01-26 15:33:20 PST
Created attachment 591969 [details] [diff] [review]
v1

This is green on try: https://tbpl.mozilla.org/?tree=Try&rev=f6dfe8025f80
No change in performance on v8.
Comment 2 Igor Bukanov 2012-01-27 08:33:49 PST
Comment on attachment 591969 [details] [diff] [review]
v1

Review of attachment 591969 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Memory.cpp
@@ +18,5 @@
> +{
> +    JS_ASSERT(size >= alignment);
> +    JS_ASSERT(size % alignment == 0);
> +    JS_ASSERT(size % PageSize == 0);
> +    JS_ASSERT(alignment % PageSize == 0);

On Windows the VirtualAlloc allocation granularity is 64K. So if one needs alignment less than that, then VirtualAlloc should not be used to avoid creating too many unusable  holes in the address space. So have a static variable that will be initialized on demand to SYSTEM_INFO::dwAllocationGranularity as returned by GetSystemInfo, http://msdn.microsoft.com/en-us/library/windows/desktop/ms724381%28v=vs.85%29.aspx , and check that the alignment is a multiple of that.

Then the special case should check that the alignment matches that and not PageSize.

Another thing is the patch should assert that PageSize matches the runtime value as returned by GetSystemInfo for sanity and similarly on UNIX-like systems.

Otherwise the patch is OK.
Comment 3 Terrence Cole [:terrence] 2012-01-27 11:11:38 PST
Excellent suggestions.  I'll re-request if my try run is green:
https://tbpl.mozilla.org/?tree=Try&rev=cf40c98b3763
Comment 4 Terrence Cole [:terrence] 2012-01-30 11:22:35 PST
Fixed various OSX breakage, trying again:
https://tbpl.mozilla.org/?tree=Try&rev=be27e0a52d02
Comment 5 Terrence Cole [:terrence] 2012-02-03 11:07:11 PST
Created attachment 594248 [details] [diff] [review]
v2: With review feedback

I let this sit for a week so I could give it another read through with a clean perspective.  It still looks okay to me (and more importantly is green on try) so I'm reposting it for a second review pass.

Let me know if I should flag additional people for detailed review of individual OS code - MacOS in particular.  MacOS has the only really new code in this patch and, unfortunately, I know next to nothing about that environment.
Comment 6 Igor Bukanov 2012-02-03 14:14:27 PST
Comment on attachment 594248 [details] [diff] [review]
v2: With review feedback

Review of attachment 594248 [details] [diff] [review]:
-----------------------------------------------------------------

gc/Memory.* does not sound as a good name for the file if it is going to be used outside the GC. Perhaps we should add a directory like os or something?

::: js/src/gc/Memory.cpp
@@ +20,5 @@
> +{
> +    SYSTEM_INFO sysinfo;
> +    GetSystemInfo(&sysinfo);
> +    if (sysinfo.dwPageSize != PageSize)
> +        return false;

The method should abort on the page mismatch. Also it should be void and called from JS_NewRuntime at place where it does if (!js_NewRuntimeWasCalled) { check to avoid useless calls per each runtime created.

@@ +31,5 @@
> +{
> +    JS_ASSERT(size >= alignment);
> +    JS_ASSERT(size % alignment == 0);
> +    JS_ASSERT(size % PageSize == 0);
> +    JS_ASSERT(alignment % PageSize == 0);

I repeat myself from the last review: the code must assert that alignment % AllocationGranularity == 0, not that alignment % PageSize - see my previous comments for reasoning.

@@ +215,5 @@
> +    JS_ASSERT(uintptr_t(p) % PageSize == 0);
> +    return true;
> +}
> +
> +#elif defined(XP_MACOSX) || defined(DARWIN)

Can we just drop the whole XP_MACOSX business and just use the generic mmap code? In bug 691731 I observed that mmap is in fact faster than vm_allocate.

::: js/src/jsgc.cpp
@@ +879,5 @@
>  
>  JSBool
>  js_InitGC(JSRuntime *rt, uint32_t maxbytes)
>  {
> +    if (!InitMemorySubsystem())

This should be called from js_NewRuntime
Comment 7 Terrence Cole [:terrence] 2012-02-03 16:30:38 PST
(In reply to Igor Bukanov from comment #6)
> gc/Memory.* does not sound as a good name for the file if it is going to be
> used outside the GC. Perhaps we should add a directory like os or something?

I initially thought that the JIT code-buffer allocator might be interested in using this as well, but that's embedded deep in assembler/assembler under JSC code.  Other than that, the only code I see using this is explicitly GC in nature.  What non-GC user do you anticipate?

> Can we just drop the whole XP_MACOSX business and just use the generic mmap
> code? In bug 691731 I observed that mmap is in fact faster than vm_allocate.

Thanks for the link!  I had forgotten about that bug.  I wish I knew how likely the OS/2 and Solaris code are to be used ever.  It would be nice to drop those as well.

New try run is going at:
https://tbpl.mozilla.org/?tree=Try&rev=8dadf5c44d69
Comment 8 Igor Bukanov 2012-02-03 16:33:52 PST
(In reply to Terrence Cole [:terrence] from comment #7)
> I initially thought that the JIT code-buffer allocator might be interested
> in using this as well, but that's embedded deep in assembler/assembler under
> JSC code.  Other than that, the only code I see using this is explicitly GC
> in nature.  What non-GC user do you anticipate?

I do not anticipate anything! I was just referring to the comment 0 - that implied that the current code can be unified with non-GC parts...
Comment 9 Terrence Cole [:terrence] 2012-02-03 16:40:27 PST
Ah, sorry!  I consider the Nursery and WriteBuffer to be GC implementation details.  I did not mean to imply that we should unify with any of the non-GC allocators (at least not in the near term).
Comment 10 Igor Bukanov 2012-02-03 16:50:23 PST
(In reply to Terrence Cole [:terrence] from comment #9)
> I consider the Nursery and WriteBuffer to be GC implementation
> details. 

Then gc/Memory.* is fine!
Comment 11 Terrence Cole [:terrence] 2012-02-08 17:41:05 PST
Created attachment 595590 [details] [diff] [review]
v3: With second round of review feedback.

Looks green on try.
Comment 12 Igor Bukanov 2012-02-09 00:25:21 PST
Comment on attachment 595590 [details] [diff] [review]
v3: With second round of review feedback.

Review of attachment 595590 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Memory.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Hm, shouldn't the first lines here and in the new header include file style hints like: 

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
 * vim: set ts=8 sw=4 et tw=78:

?
Comment 13 Terrence Cole [:terrence] 2012-02-09 10:14:13 PST
Added.

http://hg.mozilla.org/integration/mozilla-inbound/rev/aca408a1d17b
Comment 14 Ed Morley [:emorley] 2012-02-10 05:05:15 PST
https://hg.mozilla.org/mozilla-central/rev/aca408a1d17b
Comment 15 Terrence Cole [:terrence] 2012-04-12 12:17:21 PDT
*** Bug 696119 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.