571407 - Changes to enable JIT in Symbian build of Flash Player

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Samuli Kekki]

This bug contains changes and new files needed for enabling JIT in the Symbian build of Flash Player.

This bug does not contain files for Symbian avm shell. It will be another bug.

Comment 1

[Samuli Kekki]

Attachment: Changes needed to make in avm core files.

Comment 2

[Samuli Kekki]

Attachment: Changes needed to make in the existing Symbian files.

I will also add the full files to this bug in the case it is too difficult to comprehend the patch.

Comment 3

[Samuli Kekki]

Attachment: Adding MMGCPortSymbian.cpp as a full file, just in case the patch is too difficult to read.

Comment 4

[Samuli Kekki]

Attachment: Adding SymbianPortUtils.cpp as a full file, just in case the patch is too difficult to read.

Comment 5

[Samuli Kekki]

Attachment: SymbianHeap.cpp: a new file for platform\symbian. SymbianHeap allocates regular memory.

Comment 6

[Samuli Kekki]

Attachment: SymbianHeap.h: a new file for platform\symbian. SymbianHeap allocates regular memory.

Comment 7

[Samuli Kekki]

Attachment: SymbianJITHeap.cpp: a new file for platform\symbian. SymbianJITHeap allocates code memory.

Comment 8

[Samuli Kekki]

Attachment: SymbianJITHeap.h: a new file for platform\symbian. SymbianJITHeap allocates code memory.

Comment 9

[Rick Reitmaier]

Comment on attachment 450527 [details] [diff] [review]
Changes needed to make in the existing Symbian files.

VMPI_alloc()  - you're changing the semantics of this call in that its clearing out the memory in the symbian case.   This could result in performance issues, in that the memory may be cleared multiple times. 

There is also '#if 0' code in the patch.  Normally we don't like to include dead code, it will just go stale over time, but if it helps temporarily helps with debugging or some such it should be fine.

Comment 10

[Rick Reitmaier]

Comment on attachment 450525 [details] [diff] [review]
Changes needed to make in avm core files.

The CodeAlloc change is fine , but the changes to avmshell-features goes against the grain of the system, was this supposed to be submitted?

Comment 11

[Rick Reitmaier]

Attachment: consolidate all the changes

Consolidated all the changes to a single patch except for the rejected changes in /shell/avmshell-features.h from the first patch.

I suggest that those show up in a different patch and Lars should be able to provide guidance on what form they need to take as it appears that you're trying to manipulate the features system in an odd way.

I've reviewed all the changes for this patch that I'm posting and they look reasonable to me, but asking Lars to focus on the MMgc changes.   Additionally, if approved, can you please push the patch Lars, as I will be on vacation for the next few weeks.

Comment 12

[Rick Reitmaier]

Attachment: fix tabs

Teased out the non-nanojit portion of the patch and converted tabs to spaces.

Comment 13

[Rick Reitmaier]

Attachment: nanojit portion

Nanojit-only change which I will land in nanojit shortly.

Comment 14

[Rick Reitmaier]

pushed nanojit patch https://bug571407.bugzilla.mozilla.org/attachment.cgi?id=451782 to http://hg.mozilla.org/projects/nanojit-central/rev/d992fed9f916

Comment 15

[Samuli Kekki]

Hi Rick, I heard earlier from Lars, that avmshell-features.h can be edited by hand. Other than that, there can be chance to improve the patch.

Comment 16

[Lars T Hansen]

Comment on attachment 451781 [details] [diff] [review]
fix tabs

The documentation for VMPI_allocateCodeMemory states specifically that the function never returns NULL.  (It is also not a terribly good idea to have assertions in that functions rather than checks that apply even in release builds, this is a security concern.)  Please read the documentation for this and VMPI_freeCodeMemory carefully, and study the other platform implementations and take note of the error checking that is performed in those implementations.  I see that SymbianJITHeap::Alloc has a comment where it returns 0, "Shut down the flash player after this!".  It is the task of VMPI_allocateCodeMemory to do that.

This comment probably needs to be investigated: "TEMPORARY FIX: MMgc should zero initialize the memory, but it does not seem to do it in all cases."

It's not clear to me if SymbianJITHeap::Alloc is doing the right thing by returning 0 straightaway if it fails to allocate, or if it should somehow invoke the OOM machinery to try to free up some memory.  That might be something to note as follow-on work.

You mention in a comment that it might be good if GCHeap could make use of RChunk / SymbianHeap memory directly instead of RChunk / SymbianHeap simulating the reserveMemoryRegion API.  I think that's a good idea and it's a good time to be discussing it since we're doing some GCHeap work for 10.2.  Feel free to start a thread, include at least myself and fklockii.

(R- for the VMPI_allocateCodeMemory issue.)

Comment 17

[Lars T Hansen]

(In reply to comment #15)
> Hi Rick, I heard earlier from Lars, that avmshell-features.h can be edited by
> hand. Other than that, there can be chance to improve the patch.

I'm cool with what you've done to that file.  We could do it differently by adding #if AVMSYSTEM_SYMBIAN to the earlier definitions of each of the features you're trying to control here, but I don't think that makes things all that much cleaner.  The high bit of the feature system is to make the features visible; there's bound to be some mess in avmshell-features.h and avmhost-features.h in how we select features appropriately for various systems.

Comment 18

[Samuli Kekki]

Hi Lars, thanks for feed back.

>  I see that SymbianJITHeap::Alloc has a comment where it returns 0, "Shut down
> the flash player after this!".  It is the task of VMPI_allocateCodeMemory to do that.

You are right, that is missing functionality. I should somehow shut down the player if we can not allocate. Not sure on what schedule I can do this work, but I'll put it on my list todo.

> This comment probably needs to be investigated: "TEMPORARY FIX: MMgc should
> zero initialize the memory, but it does not seem to do it in all cases."

Yes, that is a point I have been going to raise. There is an API VMPI_areNewPagesDirty. It was added to the vm because Symbian returns non-zero initialized memory from its APIs. Returning true should force MMgc to zero the memory, if needed, inside MMgc. However as this method returns true only on Symbian, this functionality is not tested regularly in avm team's testing so the feature may have fallen into a broken state. At least I had some crashes on Symbian that got fixed by always zeroing the memory on Symbian platform before returning it to MMgc, whether returning true or false from this API.

So, we could either double check that MMgc is doing the right thing always when platform returns true from VMPI_areNewPagesDirty, or, remove this API because it adds overhead to avm team (because you have to maintain it although only Symbian needs it) and state the assumption that all platform APIs that return memory to MMgc should return zero initialized memory. I think all other platforms than Symbian return zero initialized memory automatically.

Needs more investigation. I'm fine with removing VMPI_areNewPagesDirty API.

Comment 19

[Lars T Hansen]

(In reply to comment #18)
> Hi Lars, thanks for feed back.
> 
> >  I see that SymbianJITHeap::Alloc has a comment where it returns 0, "Shut down
> > the flash player after this!".  It is the task of VMPI_allocateCodeMemory to do that.
> 
> You are right, that is missing functionality. I should somehow shut down the
> player if we can not allocate. Not sure on what schedule I can do this work,
> but I'll put it on my list todo.

The expedient thing may be to simply call GCHeap::Abort.  I think that's what we do elsewhere, and that mechanism is at least reasonably debugged.  And it makes sense - it's an OOM situation.

> > This comment probably needs to be investigated: "TEMPORARY FIX: MMgc should
> > zero initialize the memory, but it does not seem to do it in all cases."
> 
> Yes, that is a point I have been going to raise. There is an API
> VMPI_areNewPagesDirty. It was added to the vm because Symbian returns non-zero
> initialized memory from its APIs. Returning true should force MMgc to zero the
> memory, if needed, inside MMgc. However as this method returns true only on
> Symbian, this functionality is not tested regularly in avm team's testing so
> the feature may have fallen into a broken state.

MMgc also has to zero the pages if virtual memory isn't used, and that functionality is supported (though of course it could be buggy).  We currently do not use virtual memory on MacOS 10.4, and it may or may not be used on MIPS.  So there's at least a fighting chance that the mechanism has been debugged and is being maintained.  Since reference counting depends on zeroed memory and there are copious debug asserts to catch non-zeroed memory I'd be a little bit surprised if this feature isn't working as it's supposed to.

> At least I had some crashes on
> Symbian that got fixed by always zeroing the memory on Symbian platform before
> returning it to MMgc, whether returning true or false from this API.

OK, we should dig deeper, and soon.  Please file a bug if you have any actionable data!

> So, we could either double check that MMgc is doing the right thing always when
> platform returns true from VMPI_areNewPagesDirty, or, remove this API because
> it adds overhead to avm team (because you have to maintain it although only
> Symbian needs it) and state the assumption that all platform APIs that return
> memory to MMgc should return zero initialized memory. I think all other
> platforms than Symbian return zero initialized memory automatically.
> 
> Needs more investigation. I'm fine with removing VMPI_areNewPagesDirty API.

We can discuss that, but for the moment I think we keep it, and there are bugs in MMgc we should fix them.  Please provide what information you have.

Comment 20

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/fb3abbe6f010

Comment 21

[Nicholas Nethercote [inactive]]

TM: http://hg.mozilla.org/tracemonkey/rev/2810a5e9d4d6

Comment 22

[louiszer]

Attachment: chang to

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2810a5e9d4d6

Comment 24

[Rick Reitmaier]

Comment on attachment 450525 [details] [diff] [review]
Changes needed to make in avm core files.

removing self from review, patch has landed

Comment 25

[Rick Reitmaier]

Comment on attachment 451781 [details] [diff] [review]
fix tabs

no longer need skekki feedback...patch landed.

Comment 26

[Steven Johnson]

(In reply to comment #16)
> Comment on attachment 451781 [details] [diff] [review]
> fix tabs

> (R- for the VMPI_allocateCodeMemory issue.)

In order to get some integration going more simply, I'm going to land this (despite the R-) with some FIXME comments added; this is odious and normally unacceptable, but in this case, getting various codebases in sync trumps this issue. Apologies in advance...

Comment 27

[Steven Johnson]

pushed the R- changes as 5229:1e7048f6e23c.