Changes to enable JIT in Symbian build of Flash Player

RESOLVED FIXED in flash10.1-mobile

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: skekki, Assigned: skekki)

Tracking

unspecified
flash10.1-mobile
ARM
iOS 4

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey)

Attachments

(4 attachments, 8 obsolete attachments)

2.36 KB, patch
Details | Diff | Splinter Review
25.98 KB, patch
lhansen
: review-
Details | Diff | Splinter Review
590 bytes, patch
Details | Diff | Splinter Review
590 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 450525 [details] [diff] [review]
Changes needed to make in avm core files.
Attachment #450525 - Flags: review?(stejohns)
(Assignee)

Comment 2

8 years ago
Created attachment 450527 [details] [diff] [review]
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.
Attachment #450527 - Flags: review?(stejohns)
(Assignee)

Updated

8 years ago
Attachment #450525 - Flags: review?(stejohns) → review?(rreitmai)
(Assignee)

Updated

8 years ago
Attachment #450527 - Flags: review?(stejohns) → review?(rreitmai)
(Assignee)

Comment 3

8 years ago
Created attachment 450529 [details] [diff] [review]
Adding MMGCPortSymbian.cpp as a full file, just in case the patch is too difficult to read.
Attachment #450529 - Flags: review?(rreitmai)
(Assignee)

Comment 4

8 years ago
Created attachment 450530 [details] [diff] [review]
Adding SymbianPortUtils.cpp as a full file, just in case the patch is too difficult to read.
Attachment #450530 - Flags: review?(rreitmai)
(Assignee)

Comment 5

8 years ago
Created attachment 450531 [details] [diff] [review]
SymbianHeap.cpp: a new file for platform\symbian. SymbianHeap allocates regular memory.
Attachment #450531 - Flags: review?(rreitmai)
(Assignee)

Comment 6

8 years ago
Created attachment 450532 [details] [diff] [review]
SymbianHeap.h: a new file for platform\symbian. SymbianHeap allocates regular memory.
Attachment #450532 - Flags: review?(rreitmai)
(Assignee)

Updated

8 years ago
Attachment #450531 - Attachment description: A new file for platform\symbian. SymbianHeap allocates regular memory. It is called from MMgcPortSymbian.cpp → SymbianHeap.cpp: a new file for platform\symbian. SymbianHeap allocates regular memory.
(Assignee)

Updated

8 years ago
Attachment #450532 - Attachment description: A new file for platform\symbian. SymbianHeap allocates regular memory. It is called from MMgcPortSymbian.cpp → SymbianHeap.h: a new file for platform\symbian. SymbianHeap allocates regular memory.
(Assignee)

Comment 7

8 years ago
Created attachment 450533 [details] [diff] [review]
SymbianJITHeap.cpp: a new file for platform\symbian. SymbianJITHeap allocates code memory.
(Assignee)

Updated

8 years ago
Attachment #450533 - Attachment is patch: true
Attachment #450533 - Flags: review?(rreitmai)
(Assignee)

Comment 8

8 years ago
Created attachment 450535 [details] [diff] [review]
SymbianJITHeap.h: a new file for platform\symbian. SymbianJITHeap allocates code memory.
Attachment #450535 - Flags: review?(rreitmai)
(Assignee)

Updated

8 years ago
See Also: → bug 536223

Comment 9

8 years ago
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.
Attachment #450527 - Flags: review?(rreitmai) → review+

Updated

8 years ago
Attachment #450525 - Flags: review?(rreitmai) → review-

Comment 10

8 years ago
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

8 years ago
Created attachment 451773 [details] [diff] [review]
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.
Attachment #450527 - Attachment is obsolete: true
Attachment #450529 - Attachment is obsolete: true
Attachment #450530 - Attachment is obsolete: true
Attachment #450531 - Attachment is obsolete: true
Attachment #450532 - Attachment is obsolete: true
Attachment #450533 - Attachment is obsolete: true
Attachment #450535 - Attachment is obsolete: true
Attachment #451773 - Flags: superreview?
Attachment #451773 - Flags: feedback?(skekki)
Attachment #450529 - Flags: review?(rreitmai)
Attachment #450530 - Flags: review?(rreitmai)
Attachment #450531 - Flags: review?(rreitmai)
Attachment #450532 - Flags: review?(rreitmai)
Attachment #450533 - Flags: review?(rreitmai)
Attachment #450535 - Flags: review?(rreitmai)

Comment 12

8 years ago
Created attachment 451781 [details] [diff] [review]
fix tabs

Teased out the non-nanojit portion of the patch and converted tabs to spaces.
Attachment #451773 - Attachment is obsolete: true
Attachment #451781 - Flags: review?(lhansen)
Attachment #451781 - Flags: feedback?(skekki)
Attachment #451773 - Flags: superreview?
Attachment #451773 - Flags: feedback?(skekki)

Comment 13

8 years ago
Created attachment 451782 [details] [diff] [review]
nanojit portion 

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

Comment 15

8 years ago
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

8 years ago
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.)
Attachment #451781 - Flags: review?(lhansen) → review-

Comment 17

8 years ago
(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.
(Assignee)

Comment 18

8 years ago
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

8 years ago
(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

8 years ago
TR: http://hg.mozilla.org/tamarin-redux/rev/fb3abbe6f010
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tamarin
TM: http://hg.mozilla.org/tracemonkey/rev/2810a5e9d4d6
Whiteboard: fixed-in-nanojit, fixed-in-tamarin → fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey

Comment 22

8 years ago
Created attachment 455733 [details] [diff] [review]
chang to

Comment 23

8 years ago
http://hg.mozilla.org/mozilla-central/rev/2810a5e9d4d6
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 24

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

removing self from review, patch has landed
Attachment #450525 - Flags: review-

Comment 25

8 years ago
Comment on attachment 451781 [details] [diff] [review]
fix tabs

no longer need skekki feedback...patch landed.
Attachment #451781 - Flags: feedback?(skekki)

Comment 26

8 years ago
(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...
OS: Symbian → iOS 4

Comment 27

8 years ago
pushed the R- changes as 5229:1e7048f6e23c.
You need to log in before you can comment on or make changes to this bug.