SpiderMonkey should have own memory allocation policy on WP8

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: alessarik, Assigned: ehoogeveen)

Tracking

Trunk
mozilla39
All
Windows Phone 8
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141105223254

Steps to reproduce:





Actual results:

VirtualAlloc function is missing on WP8. HeapAlloc function returns memory without alignment. According with a lot of ASSERTION breakes start of SpiderMonkey.


Expected results:

All will be good if SpiderMonkey will have memory allocation policy on Windows Phone 8 separated with Windows desktop memory allocation policy.
Reporter

Updated

5 years ago
Component: Untriaged → JavaScript Engine
OS: Windows 8 → Windows Phone 8
Product: Firefox → Core
Hardware: x86_64 → All
So this is really annoying: 
1) HeapAlloc has no address parameter, so there's no way to tell it where to allocate.
2) It seems to do some sort of caching, so when you allocate an n-byte region and then deallocate it, it will still skip that region when you allocate a region of a different size.

This means we can't do our usual thing where we allocate a region that's definitely large enough, deallocate it, then allocate the amount we want inside that region. Even if we fill up all the space before our desired region, it will just skip past it.

So I think the only thing we can do here is to overallocate by up to 2x and return an aligned pointer from the resulting region. This means we have to track the full region somehow, or we wouldn't be able to free it. I'm working on a patch to do this with a splay tree.

(I realize we don't support Windows Phone, but this seems like a silly obstacle to me that should be fixable)
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → JavaScript: GC
Ever confirmed: true
Reporter

Comment 2

5 years ago
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #1)
We can have static Map which keep pointer to begin memory and size of allocated memory.
And in UnmapPages we can check region and deallocate pointer from Map (pointer to begin).
This is pretty rough, but it seems to work. I probably ought to split out the s/GetSystemInfo/GetNativeSystemInfo/ changes as well, but they aren't too entangled.

The main problems with this patch (aside from the unavoidable overallocation) are the additional global state, and the fact that it leaks a lock. I'm not sure how to avoid the latter - we'd probably have to add a ShutdownMemorySubsystem and call it from JS shutdown or something.
Assignee: nobody → emanuel.hoogeveen
Attachment #8529879 - Flags: feedback?(terrence)
Oh, I should mention: Aside from being unavailable on Windows Phone, GetSystemInfo seems to be the wrong thing to call on 64-bit Windows [1]. I don't think it's actually hurting anything aside from the cross-platform compatibility, but we might as well use the right thing.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ms724340.aspx
This one is a bit cleaner, although I haven't tested it yet (it does compile though).
Attachment #8529879 - Attachment is obsolete: true
Attachment #8529879 - Flags: feedback?(terrence)
Attachment #8530181 - Flags: feedback?(terrence)
Comment on attachment 8530181 [details] [diff] [review]
v2 - Extend the GC allocation functions to work on Windows Phone.

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

It's a ton of complex code to take without a champion from the requiring platform stepping up to maintain it. I think if we take Maksim's suggestion and make this a simple HashMap on the runtime that is protected by the GC lock, we'd have a lot less new code and the bar to taking it would be proportionally lower.
Attachment #8530181 - Flags: feedback?(terrence) → feedback-
Hmm, did I reinvent the wheel again? I'll take a look at how a HashMap could make this simpler. In the end I suppose this might be a policy decision, but note that the code is easy to test on Windows by simply making the top #if never hit.
Reporter

Comment 8

5 years ago
Maybe soon I can provide some part of code which works.
I just found out about the Windows-only _aligned_malloc [1] and _aligned_free [2]. Maksim, do you know if we can just use those instead?

[1] http://msdn.microsoft.com/en-us/library/8z34s9c6.aspx
[2] http://msdn.microsoft.com/en-us/library/17b5h8td.aspx
Assignee

Updated

5 years ago
Flags: needinfo?(alessarik)
Reporter

Comment 10

5 years ago
Posted patch wp8_memory_allocation.diff (obsolete) — Splinter Review
Fix provide simple memory allocation policy for Windows Phone 8
(It uses simple memory hash for remember aligned memory and non-aligned).
Attachment #8553645 - Flags: review?(terrence)
Attachment #8553645 - Flags: review?(emanuel.hoogeveen)
Reporter

Comment 11

5 years ago
Posted patch wp8_compilation.diff (obsolete) — Splinter Review
This patch allow to compile SpiderMonkey on Windows Phone 8 (ARM device).
Possibly, should been attached to another bug. Into which bug?
Attachment #8553646 - Flags: review?(terrence)
Attachment #8553646 - Flags: review?(emanuel.hoogeveen)
Reporter

Comment 12

5 years ago
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #9)
> I just found out about the Windows-only _aligned_malloc [1] and
> _aligned_free [2]. Maksim, do you know if we can just use those instead?
This functions are exists. But I don't know can mingw or another compilator build them.
Besides, can any of compiler (except cl from MS) build code to Windows Phone 8 ?
If not, than maybe we can use this function, but I tested only version with memory hash map.
Comment on attachment 8553645 [details] [diff] [review]
wp8_memory_allocation.diff

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

Your use of a HashMap is certainly a lot better than what I had, but I think we should just use _aligned_malloc and _aligned_free. I'll post a patch that does this (and is a lot more minimal).

Note that :njn also suggests this approach in bug 1121235, so we may end up using _aligned_malloc and _aligned_free always. Clearing terrence's review flag in light of that.
Attachment #8553645 - Flags: review?(terrence)
Attachment #8553645 - Flags: review?(emanuel.hoogeveen)
Attachment #8553645 - Flags: review-
Something like this.

Terrence, this also adds a dummy MapAlignedPagesLastDitch for Solaris and actually makes the jsapi-test compile there (barring additional problems). Somehow I get the feeling that no one is bothering with Solaris builds anymore.

Alessar, could you confirm if this works? (with part 2 applied as well, of course)

Try with this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b81ce2a2313

Try with this patch and enabling the non-desktop logic (for testing purposes):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54d9b20d764e
Attachment #8530181 - Attachment is obsolete: true
Attachment #8553761 - Flags: review?(terrence)
Attachment #8553761 - Flags: feedback?(alessarik)
> (with part 2 applied as well, of course)

That is, attachment 8553646 [details] [diff] [review].
Attachment #8553646 - Flags: review?(terrence) → review+
Comment on attachment 8553761 [details] [diff] [review]
v3 - Extend the GC allocation functions to work on Windows Phone.

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

I'm fine with it, assuming it works.
Attachment #8553761 - Flags: review?(terrence) → review+
Comment on attachment 8553646 [details] [diff] [review]
wp8_compilation.diff

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

I'm not really a peer on this code, but the changes look fine to me as quick compilation fixes. r=me with the nit fixed.

::: js/src/jsnativestack.cpp
@@ +48,5 @@
>      return reinterpret_cast<void*>(pTib->StackBase);
>  
> +# elif defined(_M_ARM)
> +    PNT_TIB pTib = reinterpret_cast<PNT_TIB>(NtCurrentTeb());
> +    return pTib->StackBase;

Wrap this in a static_cast<void*> like the line below please.
Attachment #8553646 - Flags: review?(emanuel.hoogeveen) → review+
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #18)
> Wrap this in a static_cast<void*> like the line below please.

The *case* below, even.
Reporter

Comment 20

4 years ago
Comment on attachment 8553761 [details] [diff] [review]
v3 - Extend the GC allocation functions to work on Windows Phone.

Look's like, such policy works fine.
Attachment #8553761 - Flags: feedback?(alessarik) → feedback+
Carrying forward r=terrence. Just a slight rebase.
Attachment #8553645 - Attachment is obsolete: true
Attachment #8553761 - Attachment is obsolete: true
Flags: needinfo?(alessarik)
Attachment #8581202 - Flags: review+
Carrying forward r+. jscrashreport.cpp appears to be gone, and I didn't see any replacement; perhaps it was vestigial. If there are any other compilation fixes, let's do them in a new bug.
Attachment #8553646 - Attachment is obsolete: true
Attachment #8581203 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cf9b6a1d22a4
https://hg.mozilla.org/mozilla-central/rev/0e6a3b389227
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.