AbstractFunction, MethodInfo, NativeMethod need shrinking

VERIFIED FIXED

Status

VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: stejohns, Assigned: stejohns)

Tracking

Details

Attachments

(9 obsolete attachments)

(Assignee)

Description

10 years ago
AbstractFunction and its descendants consume a disproportionately large portion of memory in large Flex apps. This bug will track patches designed to shrink its memory usage.
(Assignee)

Comment 1

10 years ago
Created attachment 364424 [details] [diff] [review]
Patch #1 -- collapse AbstractFunction and descendants

There's lots of churn in this patch, but basically it collapses AbstractFunction, MethodInfo, and NativeMethod into a single class (now called "MethodInfo" uniformly). This allows us to remove the virtual functions from this class, saving a pointer-sized field per instance. (native methods actually get slightly larger since there are unused fields, but they are fixed in size and vastly outnumbered by the non-native methods, typically by a factor of 10, so there is a small but useful net savings for Flex apps.)

Aside from the small memory savings, this has appeal in laying the groundwork for future, more meaty cuts in the now-more-manageable framework, and also removing a bunch of pointless and scary casting. It also privatizes access to more (but not all) of the fields in MethodInfo.

This patch also contains an experimental related change (currently disabled via VMCFG_METHODENV_IMPL32) that removes the shadow "impl32" field from MethodEnv, forcing callers to go into the MethodInfo to get the impl32. This saves a bit of memory but requires a double-load for method calls, which may or may not be a performance issue. (I won't push this patch until this change is established to be useful or not; it's included here because it was already present in this codebase.)
Attachment #364424 - Flags: review?(edwsmith)
(Assignee)

Updated

10 years ago
Attachment #364424 - Flags: review?(lhansen)

Comment 2

10 years ago
i know its a pain but the vcproj looks like every line got hammered.  Is it possible to hand edit the changes?
(Assignee)

Comment 3

10 years ago
Gah. Only change to the vcproj was to remove AbstractFunction.h/.cpp.

Comment 4

10 years ago
Normally i'm all for deleting dead or commented out code, but for caching blub in verifyEnter, Lars might want to keep it?

in initDMI(), s/MIR/JIT  (earlier change was clobbered)

fine to put impl32 first in the struct, but if you haven't profiled the other fields for frequency-of-access, might be a good idea.  what counts is grouping frequently accessed fields onto the same cache line, wherever they land in the struct.  assume cache lines are 32 or 64 bytes, although obviously they can vary.

Updated

10 years ago
Attachment #364424 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 5

10 years ago
(In reply to comment #4)
> Normally i'm all for deleting dead or commented out code, but for caching blub
> in verifyEnter, Lars might want to keep it?

Yeah, that's why I left it in. I also would like to nuke it but I presume Lars wants it there for now. Lars, please speak up if we can nuke this (or at least convert it into a comment rather than the dreaded "if 0")
 
> in initDMI(), s/MIR/JIT  (earlier change was clobbered)

will do
 
> fine to put impl32 first in the struct, but if you haven't profiled the other
> fields for frequency-of-access, might be a good idea.  what counts is grouping
> frequently accessed fields onto the same cache line, wherever they land in the
> struct.  assume cache lines are 32 or 64 bytes, although obviously they can
> vary.

Good point. Can VTune (etc) assist in this regard?

Comment 6

10 years ago
i think vtune can, but you have to spend some quality time with it to figure out how.  i was thinking just measuring access frequency using _vprof in the obvious places.  low pri.
(Assignee)

Comment 7

10 years ago
Comment on attachment 364424 [details] [diff] [review]
Patch #1 -- collapse AbstractFunction and descendants

removing lhansen from review (sorry, lars :-)

pushed to redux as changeset:   1527:55a97be51be5
Attachment #364424 - Attachment is obsolete: true
Attachment #364424 - Flags: review?(lhansen)

Comment 8

10 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > Normally i'm all for deleting dead or commented out code, but for caching blub
> > in verifyEnter, Lars might want to keep it?
> 
> Yeah, that's why I left it in. I also would like to nuke it but I presume Lars
> wants it there for now. Lars, please speak up if we can nuke this (or at least
> convert it into a comment rather than the dreaded "if 0")

Nuke it.
(Assignee)

Comment 9

10 years ago
Created attachment 365337 [details] [diff] [review]
Patch #2 -- cache various MethodInfo fields

Major changes to MethodInfo to reduce memory usage.

Primarily, a number of fields that can be reconstituted from re-parsing ABC are now split and cached in a new MethodSignature struct. This exactly analogous to the Traits-TraitsBinding relationship. Testing on decent-sized Flex apps shows a high-water memory use reduction of 300k-500k when used with a cache size of 32.

A bit more testing is necessary prior to pushing, primarily in the performance area, but initial testing has shown performance to be fine so I'm offering this patch for initial review (pending further perf testing).

I also changed the default size of the TraitsBindings cache to 32 -- it was formerly unlimited, but it seems clear at this point that 32 is a better default.

Also rewrote methodInfo::boxArgs to use BuiltinType instead of repeated if-else.

Also added a "miscdir" target to performance/testconfig.txt, as passing "misc" would match against "misc-mobile" and try to run the canaries.
Attachment #365337 - Flags: review?(edwsmith)
(Assignee)

Comment 10

10 years ago
FYI, there's definitely room for further improvement on the caching front as well -- we should be able to move more fields into the MethodSignature (I just did some low-hanging ones for the initial implementation).

Comment 11

10 years ago
Why is the modification to performance/testconfig.txt required? The normal -config=misc is supposed to run the canaries section. The misc-mobile config was created in order to NOT run the canaries.
(Assignee)

Comment 12

10 years ago
I wanted to have a config that would run the tests in the "misc" directory, but not the canaries. If misc-mobile will do this for me, I'll use that.
(Assignee)

Comment 13

10 years ago
Created attachment 365576 [details] [diff] [review]
Patch #2a -- cache various MethodInfo fields

Basically the same as the previous patch, but with various hotspots smoothed out so that performance is now on a par with previous builds. I need to run a full performance test suite overnight but anticipate this patch being fine performance-wise, so this patch is ready for a real review.
Attachment #365337 - Attachment is obsolete: true
Attachment #365576 - Flags: review?(edwsmith)
Attachment #365337 - Flags: review?(edwsmith)
(Assignee)

Comment 14

10 years ago
Created attachment 365577 [details] [diff] [review]
Patch #2b

Oops, Patch 2a was only a partial diff -- this should be the complete one (including all of original Patch 2 as well)
Attachment #365576 - Attachment is obsolete: true
Attachment #365577 - Flags: review?(edwsmith)
Attachment #365576 - Flags: review?(edwsmith)
(Assignee)

Comment 15

10 years ago
Created attachment 365760 [details] [diff] [review]
Patch #2c

Another revision to Patch #2, with a few more performance tweaks, and few more fields moved into the cache (yielding another 50k-100k savings).
Attachment #365577 - Attachment is obsolete: true
Attachment #365760 - Flags: review?(edwsmith)
Attachment #365577 - Flags: review?(edwsmith)

Updated

10 years ago
Attachment #365760 - Flags: review?(edwsmith) → review+

Comment 16

10 years ago
Comment on attachment 365760 [details] [diff] [review]
Patch #2c

overall:

patch should have been three patches, at least:

1. refactoring: flag/field accessor functions, cleanups
2. caching stuff (meat)
3. optimizations

The chances of finding something interesting in the review would be higher this way.  I can't R- now because it's so much more work to split them apart.

Developing two patches in parallel (first one being purely cleanup) is straightforward with named branches or separate repos, and makes reviewing & merging MUCH easier.  the downstream worked saved is greater than the upfront work to keep the independent changes separate.

detail:

AbcParser.cpp: 390 - what happened to VerboseVerify flag?
AbcParser.cpp: 486 - ditto for IS_GETTER and IS_SETTER?
AbcParser.cpp: 520 - and FINAL and OVERRIDE?

i bet these moved to the parser for MethodSignature, but haven't found them yet.  if they weren't used, do we have bugs/tests?  at least final/override better be used!  comments would help, if they are parsed later somewhere else.

Interpreter.h:64 -  ...  method->method->getMethodSignature ...
                   
is "method->method->" a typo?

Traits.cpp:1830 - need some comments about update_max_stack()... presumably this is in case the generated init code uses more stack than we started with?

Verifier.cpp:90 - verify check for invalid frame size components moved.... where?
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> (From update of attachment 365760 [details] [diff] [review])
> overall:
> 
> patch should have been three patches, at least:
> 
> 1. refactoring: flag/field accessor functions, cleanups
> 2. caching stuff (meat)
> 3. optimizations
> 
> The chances of finding something interesting in the review would be higher this
> way.  I can't R- now because it's so much more work to split them apart.

Point is taken -- I'll definitely endeavor to do this in the future. I can split them into separate patches if you'd like. 
 
> detail:
> 
> AbcParser.cpp: 390 - what happened to VerboseVerify flag?

Nuked; I thought we had discussed that this was obsolete. If not, I can trivially re-add.

> AbcParser.cpp: 486 - ditto for IS_GETTER and IS_SETTER?

Now handled in MethodInfo::setKind()

> AbcParser.cpp: 520 - and FINAL and OVERRIDE?

These bits were write-only: we set them but no code anywhere (even in Flash) ever examined them.

> i bet these moved to the parser for MethodSignature, but haven't found them
> yet.  if they weren't used, do we have bugs/tests?  at least final/override
> better be used!  comments would help, if they are parsed later somewhere else.

You say they better be used, and yet, they were not. 

> Interpreter.h:64 -  ...  method->method->getMethodSignature ...
> 
> is "method->method->" a typo?

Nope: The first "method" is the MethodEnv* argument, the second "method" is its MethodInfo* field.

> Traits.cpp:1830 - need some comments about update_max_stack()... presumably
> this is in case the generated init code uses more stack than we started with?

yep. will do.

> Verifier.cpp:90 - verify check for invalid frame size components moved....
> where?

MethodInfo::resolveSignature() -- will add comments to that effect.

Comment 18

10 years ago
are you adding comments and/or new bugs to track those write-only bits?
(Assignee)

Comment 19

10 years ago
(In reply to comment #18)
> are you adding comments and/or new bugs to track those write-only bits?

I wasn't planning on it.
(Assignee)

Comment 20

10 years ago
(In reply to comment #19)
> (In reply to comment #18)
> > are you adding comments and/or new bugs to track those write-only bits?
> 
> I wasn't planning on it.

But it sounds like you think we should. Perhaps just to investigate how it is that they have (apparently) never been used?

Comment 21

10 years ago
yes, most definitely.
(Assignee)

Comment 23

10 years ago
Comment on attachment 365760 [details] [diff] [review]
Patch #2c

pushed to redux as changeset:   1577:48ec953a1c0b
Attachment #365760 - Attachment is obsolete: true
(Assignee)

Comment 24

10 years ago
Created attachment 369196 [details] [diff] [review]
Patch #3 -- refactoring work

Some refactoring prior to further MethodInfo shrinkage: privatize the Pool::cinit list, and change it to a list of Traits (rather than MethodInfo) as this is the form that most callers actually want in the first place.
Attachment #369196 - Flags: review?(lhansen)
(Assignee)

Comment 25

10 years ago
Created attachment 369198 [details] [diff] [review]
Patch #4 -- refactoring work

Yet more refactoring. Privatize Pool:methods and Pool:method_name_indices.
Attachment #369198 - Flags: review?(lhansen)
(Assignee)

Comment 26

10 years ago
Created attachment 369202 [details] [diff] [review]
Patch #5 -- move DebuggerMethodInfo to the PoolObject

DebuggerMethodInfo is only allocated when there is a debugger instantiated, so instead of reserving space in the MethodInfo for it, move it into a List in the PoolObject that is only allocated and filled in when there is a debugger. Saves a pointer-sized field from each MethodInfo in Debugger builds.
Attachment #369202 - Flags: review?(lhansen)
(Assignee)

Updated

10 years ago
Attachment #369196 - Attachment is patch: true
Attachment #369196 - Attachment mime type: application/octet-stream → text/plain

Comment 27

10 years ago
Does patch #5 get rid of those duplicate fields (IIRC local_count and scopes) that are in DMI?
(Assignee)

Comment 28

10 years ago
Created attachment 369204 [details] [diff] [review]
Patch #6 -- move abc_code_start into MethodSignature

abc_code_start is trivially calculated as a by-product of building a MethodSignature, so let's cache it there rather than keeping it around forever in MethodInfo. (only helps ABC_INTERPRETER mode)
Attachment #369204 - Flags: review?(lhansen)
(Assignee)

Comment 29

10 years ago
(In reply to comment #27)
> Does patch #5 get rid of those duplicate fields (IIRC local_count and scopes)
> that are in DMI?

Nope. But that should be done at some point too.

Comment 30

10 years ago
Comment on attachment 369196 [details] [diff] [review]
Patch #3 -- refactoring work

I think atomToPos is broken on 64-bit systems because urshift casts its first operand to uint32_t, so you may lose information here if that operand is indeed a pointer.  (Was broken before too.)
Attachment #369196 - Flags: review?(lhansen) → review+

Comment 31

10 years ago
Comment on attachment 369198 [details] [diff] [review]
Patch #4 -- refactoring work

Über-nit: methodCount() is const so it's not actually necessary to factor it out like you do three or four places (but I would have done it too, probably).
Attachment #369198 - Flags: review?(lhansen) → review+

Updated

10 years ago
Attachment #369202 - Flags: review?(lhansen) → review+

Comment 32

10 years ago
Comment on attachment 369202 [details] [diff] [review]
Patch #5 -- move DebuggerMethodInfo to the PoolObject

Every call to dmi() is followed by an assert that checks that the value was not NULL.  Move the assert into dmi()?

Also, in several cases code follows the assert that tests whether dmi is not NULL.  Seems redundant.

With this much uncertainty the behavior of dmi() deserves a comment in the header.

Updated

10 years ago
Attachment #369204 - Flags: review?(lhansen) → review+

Comment 33

10 years ago
Comment on attachment 369204 [details] [diff] [review]
Patch #6 -- move abc_code_start into MethodSignature

I'm not overly fond of the 

#ifdef FOO
#else
...
#endif

structure to replace ifndef, although I admit it's hard to misread it.
(Assignee)

Comment 34

10 years ago
(In reply to comment #30)
> (From update of attachment 369196 [details] [diff] [review])
> I think atomToPos is broken on 64-bit systems because urshift casts its first
> operand to uint32_t, so you may lose information here if that operand is indeed
> a pointer.  (Was broken before too.)

atomToPos() is some wonky pre-existing thing where we store an offset as an "atom" -- you're right, this does look incorrect, but a pre-existing incorrectness. I'll open a new bug on it.

(In reply to comment #32)
> (From update of attachment 369202 [details] [diff] [review])
> Every call to dmi() is followed by an assert that checks that the value 
> was not NULL.  Move the assert into dmi()?

will do.

> Also, in several cases code follows the assert that tests whether dmi is not
> NULL.  Seems redundant.

dmi actually *can* be null (for MethodInfo's that are synthesized by genInitBody), but in practice we should never call dmi() for these. The null-checking is paranoia for debugger builds, on the theory that crashing is bad.
 
> With this much uncertainty the behavior of dmi() deserves a comment in the
> header.

Will do.


(In reply to comment #33)
> (From update of attachment 369204 [details] [diff] [review])
> I'm not overly fond of the 
> 
> #ifdef FOO
> #else
> ...
> #endif
> 
> structure to replace ifndef, although I admit it's hard to misread it.

Yeah, for me this is a pick-your-poison. It's ugly, but I've misread #ifndef's one too many times, so I prefer the ugly-but-clear over the less-ugly-but-subtle approach. If you are allergic to this style, I'll change it.
(Assignee)

Comment 35

10 years ago
Comment on attachment 369196 [details] [diff] [review]
Patch #3 -- refactoring work

pushed to redux as changeset:   1634:deabe0272a1e
Attachment #369196 - Attachment is obsolete: true
(Assignee)

Comment 36

10 years ago
Comment on attachment 369198 [details] [diff] [review]
Patch #4 -- refactoring work

pushed to redux as changeset:   1634:deabe0272a1e
Attachment #369198 - Attachment is obsolete: true
(Assignee)

Comment 37

10 years ago
Comment on attachment 369202 [details] [diff] [review]
Patch #5 -- move DebuggerMethodInfo to the PoolObject

pushed to redux as changeset:   1634:deabe0272a1e
Attachment #369202 - Attachment is obsolete: true
(Assignee)

Comment 38

10 years ago
Comment on attachment 369204 [details] [diff] [review]
Patch #6 -- move abc_code_start into MethodSignature

pushed to redux as changeset:   1634:deabe0272a1e
Attachment #369204 - Attachment is obsolete: true
(Assignee)

Comment 39

10 years ago
I think this bug is as done as it's gonna be for a while -- recommend we close it as fixed.
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 40

9 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.