ABC strings are currently dynamic, and copied during creation. If they are ASCII, they can be static, saving a lot of memory space.
Rob Borcic implemented static string creation from ABC data (see ConstDataScriptBufferImpl in ScriptBuffer.h) and Tamarin is using this for its own builtin ABCs. Its use may need to be propagated into host code for host builtins, and host code that does not require ABCs to be unloaded may wish to use it for dynamically loaded ABCs, but that's properly outside the scope of this bug. If this bug is about something more than that then please note it here; otherwise I think the bug can be closed.
ConstDataScriptBufferImpl may not (or not in its whole) be needed - I will have to look at the code. The StringObject class handles static string data just fine. For unloading ABC data, we need another routine that goes over the table of static String* and calls makeDynamic() on each of them. I would like to make this change ASAP after the discussion about unloading ABC data has settled. We should leave this bug open.
Noted. Please ask me for code review on all proposed changes.
Created attachment 380401 [details] [diff] [review] Suggested fix This is a suggested fix. Just pass in true instead of isConstant to internUTF8(). I have added a @todo that described what to do if unloading of SWFs should be supported. If the fix is acceptable, I think that the various ScriptBufferImpl classes could be cleaned up as well, and the isConstant member could be removed.
Attachment #380401 - Flags: review?(lhansen)
Comment on attachment 380401 [details] [diff] [review] Suggested fix Not acceptable as we don't know what the consequences are - unloading is already supported in some form, so this change would probably introduce a regression. Let's take this to e-mail. Include Steven, Edwin, and myself.
Priority: -- → P2
Target Milestone: --- → flash10.x
Created attachment 382951 [details] [diff] [review] First step: add System.collect() This is the first of a series of patches. To have an automated way of testing memory usage, I have implemented System.collect(force:Boolean=false):Number as Lars suggested. The "force" argument is currently ignored, and the return value is the number of bytes freed. The patch does not include the auto-generated files shell_toplevel.abc/cpp/h. I will generate these files after the patch has been approved and submit them. Follow-up patches will define test cases before I start to work on the code.
Comment on attachment 382951 [details] [diff] [review] First step: add System.collect() The return value doesn't make sense if you take the 'force' argument into account: if force is false then it should be legitimate to ignore the call. A better return type would be boolean, returning 'true' if the collection finished (and sweeping was performed) and 'false' if not. This is no loss of generality, as totalMemory is already available via the System class.
Attachment #382951 - Flags: review?(lhansen) → review-
Comment on attachment 382951 [details] [diff] [review] First step: add System.collect() what is the eventual purpose of the "force" parameter? ie, when/why would you want to call this with force=false?
Maybe this? If (force) core()->GetGC()->Collect(); else core()->GetGC()->QueueCollection(); Lars, Steven, please agree on an API and document the API you want here before I continue.
then might I suggest: System.collect() System.queueCollection() (you guys decide, just an idea to make the api cleaner)
(In reply to comment #10) > then might I suggest: > > System.collect() > System.queueCollection() +1
Return values? Both GC methods are void, so what would you like to see?
how about "void"?
Michael's suggestion is not appropriate. The idea of having the force parameter is that if it is true, a synchronous collection is forced (to completion); if it is false, the collector is given the hint that this may be a useful time to start, advance, or finish an incremental collection but it is under no compulsion to do so. Specifically it does not "queue a collection", not that I'm sure what that would mean. (The reason for having this mode is that forcing a collection is usually a really bad idea.) However, given the debate I suggest that we just remove the parameter for now. Since the System class is specific to the avmshell and can be changed whenever we feel like it, we should not speculate about the kinds of parameters that the function might need to take in the future, as needs (and hopefully the GC) evolve.
On reflection I think the better name and signature for the function are void System.forceFullCollection()
Created attachment 383272 [details] [diff] [review] System.collect() and System.queueCOllection() As discussed in the thread. Again, the patch does not contain auto-generated files.
Created attachment 383273 [details] [diff] [review] System.forceFullCollection() and System.queueCollection() Forgot to rename collect() to forceFullCollection().
Attachment #383273 - Flags: superreview?(stejohns) → superreview+
Patch pushed as changeset 2026:f411eef033d7. Moving on...
Created attachment 387632 [details] [diff] [review] 1st attempt for a patch This is an email that I sent out today 03:25AM PST. I decided to attach the patch anyway, because time is valuable. My patch (including lazy ABC string initialization) is almost ready to go – I just submitted a sandbox build to verify its stability. Who would the r+ and sr+ be? Please advise – Dan is pushing hard to get this in before next week’s tr->tc integrate. Update: The sandbox tests show an unexpected pass for WinMobile: as3/ShellClasses/toplevel.abc : unexpected pass: System.privateMemory >0 = true PASSED! reason: bug https://bugzilla.mozilla.org/show_bug.cgi?id=459851 Don’t know what this would mean. Currently, I cannot find a way to have the PoolObject of an additionally loaded file to go away. I strongly suspect a circular reference – the Traits objects stored in the PoolObject’s _scripts List reference the PoolObject. Insofar, I cannot test the “dynamization” of static strings when the PoolObject goes away. OTOH, the code is trivial and should execute well (it does not now, because it never executes during shutdown of the GC). I created a four performance test cases: 1) Define 10000 static ASCII strings of 50 characters each, access all of them 2) Define 10000 static ASCII strings of 50 characters each, access half of them 3) Define 10000 static Latin-1 strings of 50 characters each, access all of them 4) Define 10000 static Latin-1 strings of 50 characters each, access half of them The memory usage shows as follows: language/string/static_ascii_array_100.as 6.1M 5.0M -18.0 memory language/string/static_ascii_array_50.as 6.0M 4.5M -25.0 memory language/string/static_latin1_array_100.as 7.6M 7.9M 3.9 memory language/string/static_latin1_array_50.as 7.5M 6.9M -8.0 memory which is as expected. Some of the language/string tests appear to use more memory, but this is due to the fact that appending to a static string happens in different increments than to the same dynamic string, simply because the dynamic string already comes with extra characters at the end and therefore meets a different size increase threshold. The execution timing of all performance tests varies between +- 3%, which is also as expected (even with 10 iterations), since the timer apparently is based on the Date object, so there is no detectable performance penalty. The acceptance test suite takes the same time with the patch.
Created attachment 387633 [details] Zipped performance tests This set of tests go into performance/language/string. Please change the reviewer chain to your liking.
Sandbox unexpected pass is expected. The failure was fixed in 2099:9220066e5d01 and then the "expectedfail" was removed in rev 2101:9d6892fe7ce6
Comment on attachment 387632 [details] [diff] [review] 1st attempt for a patch looks good. super-nit: we should prefer "uint8_t" to "byte" for new code. possible tweaky optimization: in getString() we always have to check if (dataP->abcPtr >= _abcStringStart && dataP->abcPtr < _abcStringEnd) could we use the classic range-check optimization, which relies on the fact that (x >= min && x < max) is equivalent to (unsigned)(x-min) < (max-min) thus allowing only a single compare?
Attachment #387632 - Flags: review?(stejohns) → review+
Attachment #387633 - Flags: superreview?(stejohns) → superreview+
Will add super-nit and range-check optimization: Replace PoolObject::_abcStringEnd with _abcStringSize Store the size instead of the end in AbcParser.cpp: pool->_abcStringSize = (uint32_t) (pos - pool->_abcStringStart); change the compare to if ((uint32_t) (dataP->abcPtr - _abcStringStart) < _abcStringSize)
Should include a set of tests that are typed, all other performance/language/string tests have both typed and untyped versions.
pushed to redux as changeset: 2119:a24fd0ef7bc4
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Created attachment 388981 [details] [diff] [review] Patch for bugs Patch had a few bugs that subsequent testing turned up: -- makeDynamic was using wrong address for WBRC -- makeDynamic shouldn't be called for empty strings -- pool string for index 0 was never initialized (should be empty string)
Assignee: daumling → stejohns
Status: RESOLVED → REOPENED
Attachment #388981 - Flags: review?(edwsmith)
Resolution: FIXED → ---
pushed as changeset: 2150:8207c82df001
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago → 9 years ago
Resolution: --- → FIXED
Created attachment 389051 [details] [diff] [review] Optimization The existing code works, but can re-allocate many more strings than necessary: since all pool strings are interned, ABC/SWF that are loaded after the builtins may share strings with the pool from the builtins (e.g., "Array"). There's no point in making such a string dynamic, since its ABC data isn't going away. Solution is to pass the ABC start/end range into makeDynamic and only dynamic-ize strings that point to data in that range. Also tweaked the pointer-in-range calculations as suggested in earlier comments.
Attachment #389051 - Flags: review?(edwsmith)
changeset 2152:2528e2d94a90 test/performance/language/string/static_latin1_array* was producing 356 errors when compiling, saving the .include file as UTF-8 fixes the issue
Created attachment 389148 [details] [diff] [review] Patch to disable static strings Hastily adding this patch (r=me): MMgc doesn't reliably support allocations from a finalizer (which is required for this to work). Disabled for now; we either need to make MMgc support this reliably or redesign this feature to dynamic-ize the strings elsewhere.
Attachment #389148 - Flags: review+
I believe that Steven is wrong in comment #29. PoolObject::getString() uses AvmCore::internStringUTF8() to create the dynamic string. If the string in question was a string that already had been loaded (like Array or void), the returned string would not be a new string, but the one found in the interned strings hash table (which would be dynamic for additional loaded ABC code). It would be a good idea to optimize AvmCore::internStringUTF8() to not pre-allocate a string (this is where the excess allocations take place). I have created bug 505194 for this patch.
Created attachment 389455 [details] [diff] [review] Re-introduced the isConstant() check for builtin ABC strings Reopened the bug and assigned to myself because the story is not fully over yet. This patch re-introduces the isConstant() flag that is true for builtin ABC code. The flag is stored in PoolObject and used to create constant strings for builtin ABC images that are never unloaded. The code was active before, but was removed with the first static strings patch, so it is tested and tried. Also introduced nit fixes as described in comment #23. Set r? to lhansen because stejohns is OOTO.
Comment on attachment 389455 [details] [diff] [review] Re-introduced the isConstant() check for builtin ABC strings This change to PoolObject.cpp is wrong, because it changes the sense of the test from "pointer outside the range between start and end" to "pointer inside the range of start and end": @@ -91,17 +92,17 @@ - if (dataP->abcPtr < _abcStringStart || dataP->abcPtr >= _abcStringEnd) + if ((uint32_t) (dataP->abcPtr - _abcStringStart) < _abcStringSize) The change further down is correct, however. Apart from that this looks good.
Attachment #389455 - Flags: review?(lhansen) → review-
(Oh what nasty formatting. Trying again:) @@ -91,17 +92,17 @@ ... - if (dataP->abcPtr < _abcStringStart || dataP->abcPtr >= _abcStringEnd) + if ((uint32_t) (dataP->abcPtr - _abcStringStart) < _abcStringSize)
Created attachment 389473 [details] [diff] [review] Fixed the bug that Lars detected The code is currently not used, but rather kept there as a reminder about what to do. Nevertheless, the code must be correct.
(In reply to comment #32) > I believe that Steven is wrong in comment #29. PoolObject::getString() uses > AvmCore::internStringUTF8() to create the dynamic string. If the string in > question was a string that already had been loaded (like Array or void), the > returned string would not be a new string, but the one found in the interned > strings hash table (which would be dynamic for additional loaded ABC code). Correct. But when the secondary ABC is unloaded, it was dynamic-izing all strings from its pool; if this was an interned string "inherited" from a builtin pool, it would pointlessly dynamic-ize it.
Comment on attachment 389148 [details] [diff] [review] Patch to disable static strings Since this patch was pushed, MMgc has apparently been fixed to guarantee allocations from a finalizer, so we should re-enable this.
Comment on attachment 389473 [details] [diff] [review] Fixed the bug that Lars detected Dumb question: why not use the PTR_IN_RANGE() macro introduced earlier?
(In reply to comment #33) > This patch re-introduces the isConstant() flag that is true for builtin ABC > code. The flag is stored in PoolObject and used to create constant strings for > builtin ABC images that are never unloaded. The code was active before, but was > removed with the first static strings patch, so it is tested and tried. Sorry for my confusion (first day back from vacation)... I don't understand why we need (or want) the isConstant() flag, assuming we can re-enable USE_STATIC_ABC_STRINGS correctly.
The macro is nice, but it involves a subtract operation that can be avoided if the size is stored and compared against. You are right - we do not need isCOnstant() if USE_STATIC_ABC_STRINGS is re-enabled.
(In reply to comment #41) > The macro is nice, but it involves a subtract operation that can be avoided if > the size is stored and compared against. You are correct. Nice! > You are right - we do not need isCOnstant() if USE_STATIC_ABC_STRINGS is > re-enabled. OK, good, now I understand. I don't want to re-enable USE_STATIC_ABC_STRINGS until I've had a chance to vet it in Flash/AIR, but if it passes muster there, I vote let's keep the isConstant() stuff out in the name of simplicity. (Until then, it's a nice change.)
We should re-introduce Steven's change to String::makeDynamic(), where he passes in the start pointer and the size of the static ABC string data that is about to be unloaded. This prevents unnecessary string dynamization.
Comment on attachment 389051 [details] [diff] [review] Optimization looks like this has been overcome by other patches. if not, rebase and re- R?
Attachment #389051 - Flags: review?(edwsmith) → review-
(In reply to comment #43) > We should re-introduce Steven's change to String::makeDynamic(), where he > passes in the start pointer and the size of the static ABC string data that is > about to be unloaded. This prevents unnecessary string dynamization. Yeah, probably. Let's hold off on these patches till I get a chance to torture-test it in Flash/AIR, probably today or tomorrow.
Torture-testing the makeDynamic patch ("Optimization" above) still fails in Flash -- we end up dying with bogus string data, so clearly there's a latent bug somewhere. Needs more debugging, so I'm leaving it disabled for now.
Created attachment 392240 [details] [diff] [review] Removed pointers to master strings in substrings Finally found the problem - it was an optimizer bug on x64 systems, where the pointer to the string to be appended got lost too early. - Dependent strings now store an offset into the master string instead of a pointer into the master string data. - Optimization: Interned strings are not always converted if they are dependent. They are only converted if there is a memory gain to be expected (i.e. the freed master string would release more memory than a new string would need). - Optimization: append(): If "this" is empty and only one character is appended, return core->cachedChars[x] if appropriate (charCodeAt() optimization) Steven, who should sr this patch? Is Edwin back?
Comment on attachment 392240 [details] [diff] [review] Removed pointers to master strings in substrings conceptually, this sounds good. (I have only done a quick skim, not a full review.) edwsmith should probably sr. this change will have to be torture-testsed in Flash/AIR before we can push. > "may be converted to a dynamic string, which would leave BLAH" uh... BLAH?
Comment on attachment 392240 [details] [diff] [review] Removed pointers to master strings in substrings er... BLAH == // may be converted to a dynamic string, which would leave an interior // pointer to the static content in the string instance.
This patch is still crashy, and I think I've figured out why... not sure of the best fix. The problem is that the makeDynamic code assumes that all static-abc strings live in the owner Pool's table of strings. This is not true: String::substring has an optimization for static strings: // for static strings, create a new static string pointing to the substring // no need to create a dependent string and block the master This means we can create an arbitrary number of strings that point directly into ABC data that aren't tracked anywhere. The obvious, simple fix for the crashiness is to disable this optimization (and other similar ones that I may have missed), but I'm not sure of the implications of simply disabling it. The sneakier fix would be to determine if the master string in this case is constant, and disable the optimization only for those, but AFAIK there's no way to determine that currently. Perhaps we could use a bit in m_bitsAndFlags to record the safe-to-use-for-substring status (assuming any bits are unused)?
I agree. That optimization in substring() must go. The trick is to keep one static string per string buffer, and the optimizationm violates that "trick". It does not make much sense anyway, as static ABC strings are blocked anyway inside PoolObject. Only static string that result from C++ source code may be blocked, but the memory used by these (mostly temporary) strings cannot be that much. Would you like me to submit a new patch with that optimization removed, or do you want to just comment it out?
Initial testing with that optimization out looks promising -- I'll submit a new patch with that (and some other changes) in a bit if further testing pans out.
Created attachment 393618 [details] [diff] [review] Removed pointers to master strings in substrings, fix static strings I think this addresses the issues with static strings: in addition to Michael's previous patch (which this supersedes), it changes the way string-dynamicizing (is that a word?) is handled. Rather than fix the static strings from within ~PoolObject, we now keep a list of live pools in AvmCore and handle the static->dynamic conversion in AvmCore::presweep(). [This latter change is done out of possible paranoia; though it is believed that MMgc now allows allocation from within finalizers, there is anecdotal evidence that this may still be problematic. Voodoo programming FTW!]
Attachment #393618 - Flags: review? → review?(daumling)
Comment on attachment 393618 [details] [diff] [review] Removed pointers to master strings in substrings, fix static strings Looks good.
Attachment #393618 - Flags: review?(daumling) → review+
Created attachment 393964 [details] [diff] [review] String speed optimizations Grab-bag of minor String optimizations from profiling E4X parsing. None of these individually is a huge needle-mover but put together they make a measurable improvement on XML parsing: -- change values for Width enum (k8, k16) to allow using bit-shift rather than multiply and divide. (As a nice side-effect, the width now fits into a single bit in bitsAndFlags rather than two.) -- add isStatic() and isDynamic() methods that take advantage of the bitfield properties (allows us to shave an instruction off these two operations, which in turn allows Pointers ctor to actually inline under gcc) -- isSpace is inlined and calculated with only bit operators now, eliminating several branches -- use FASTCALL protocol for StringIndexer -- several memory comparison loops were consolidated into the local "mem_equal" function. (This should possible use VMPI_memcmp instead... or better yet, a Boyer-Moore implementation) -- split matchesLatin1() into caseless and case-sensitive versions (since version desired rarely varies at runtime); optimize case-sensitive version -- add indexOfCharCode() to optimize for calling indexOf() with a string of length 1
Comment on attachment 393964 [details] [diff] [review] String speed optimizations This patch has promise but profiling against the performance tests shows some regressions that would need further investigation. Removing review-request until/unless I get time to do so.
Comment on attachment 393964 [details] [diff] [review] String speed optimizations marking obsolete; the useful bits from this have been pushed elsewhere.
Attachment #393964 - Attachment is obsolete: true
Created attachment 396008 [details] [diff] [review] Revised version of 393618 This is a heavily revised version of patch 393618 that corrects the crashiness and also improves speed back to plausible levels. This comment is short (it's late on a Friday) and I will augment with MUCH more detail on Monday, but this patch has had a lot of torture testing and is ready to be reviewed... so I wanted to get it submitted to take advantage of time zone differences in hopes of getting some initial feedback on it Monday.
Assignee: daumling → stejohns
Attachment #383273 - Attachment is obsolete: true
Attachment #387632 - Attachment is obsolete: true
Attachment #387633 - Attachment is obsolete: true
Attachment #388981 - Attachment is obsolete: true
Attachment #389051 - Attachment is obsolete: true
Attachment #393618 - Attachment is obsolete: true
Attachment #396008 - Flags: superreview?(daumling)
Attachment #396008 - Flags: review?(edwsmith)
Attachment #393618 - Flags: superreview?(edwsmith)
Attachment #396008 - Flags: superreview?(daumling) → superreview+
Comment on attachment 396008 [details] [diff] [review] Revised version of 393618 I'll jump ahead and sr this piece due to time constraints. PoolObject.cpp: Nit in line 137: uint32_t(_abcStringEnd - _abcStringStart) could be moved outside the loop. line 949: Stringp volatile rightStrKeeper = rightStr; I have tried this before to have the compiler nail the pointer and not reuse the location, but it failed to do so. Volatile is Very Unreliable. This is why I used in_ptrs2 and a MUST USE comment in the original patch. If you think that this is reliable enough, I would not object, though. Very clever use of templatized functions.
(In reply to comment #59) > line 949: Stringp volatile rightStrKeeper = rightStr; > I have tried this before to have the compiler nail the pointer and not reuse > the location, but it failed to do so. Volatile is Very Unreliable. This is why > I used in_ptrs2 and a MUST USE comment in the original patch. If you think that > this is reliable enough, I would not object, though. Well, in the specific cases I've tested it in, it has worked (and inspection of assembly code confirms this)... that said, we may just be getting lucky. My understanding of "volatile" is that it should be precisely what we need and want in this case (ie, the compiler isn't allowed to eliminate load/store). Do I misunderstand? Or is the MSVC 64-bit compiler perhaps not treating "volatile" in the way I would expect?
Another one; In lines 836 and 840 of StringObject.cpp, p should be cast to const uint8_t*; Jerry Hall reported a bug that String::containsLatin1("\xAD") fails due to signed/unsigned mismatch.
(In reply to comment #61) > Another one; In lines 836 and 840 of StringObject.cpp, p should be cast to > const uint8_t*; Jerry Hall reported a bug that String::containsLatin1("\xAD") > fails due to signed/unsigned mismatch. He's entered a separate bug on this: https://bugzilla.mozilla.org/show_bug.cgi?id=512700
Comment on attachment 396008 [details] [diff] [review] Revised version of 393618 This patch is too unwieldy to review as-is. I'm going to pick it into smaller bits (especially various optimizations) and submit as separate bugs, then re-submit a smaller version that's manageable.
Created attachment 397115 [details] [diff] [review] Revise dependent strings to use pointer-plus-offset This is a revised, partial version of Michael's previous patch. It reworks dependent strings so that they store a pointer-and-offset into the master string (rather than a direct pointer, as before); this will allow us to do static->dynamic string conversion without worrying about dynamic strings going bad. performance tests on MacTel32 show no significant performance difference before and after (within the +- 1% noise range typical on my machine). NOTE: this patch is done against a version of TR with all of the patches from https://bugzilla.mozilla.org/show_bug.cgi?id=512740 applied.
Created attachment 397138 [details] [diff] [review] Enable static-abc-strings Enable static-abc-strings. Additive to previous patch. Tested in Flash and everything seems dandy. NOTE: this patch is done against a version of TR with all of the patches from https://bugzilla.mozilla.org/show_bug.cgi?id=512740 applied.
Comment on attachment 397115 [details] [diff] [review] Revise dependent strings to use pointer-plus-offset pushed as part of changeset: 2423:de974974e963
Comment on attachment 397138 [details] [diff] [review] Enable static-abc-strings pushed as part of changeset: 2423:de974974e963
I think one is finally nailed. Closing.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago → 9 years ago
Resolution: --- → FIXED
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.