Make ABC strings static String instances

VERIFIED FIXED in flash10.1

Status

P2
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: daumling, Assigned: stejohns)

Tracking

unspecified
flash10.1
Bug Flags:
flashplayer-qrb +

Details

Attachments

(17 obsolete attachments)

(Reporter)

Description

9 years ago
ABC strings are currently dynamic, and copied during creation. If they are ASCII, they can be static, saving a lot of memory space.

Comment 1

9 years ago
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.
(Reporter)

Comment 2

9 years ago
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.

Comment 3

9 years ago
Noted.  Please ask me for code review on all proposed changes.
(Reporter)

Comment 4

9 years ago
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)

Updated

9 years ago
Attachment #380401 - Flags: review?(lhansen) → review-

Comment 5

9 years ago
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.

Updated

9 years ago
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.x
(Reporter)

Comment 6

9 years ago
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.
Attachment #380401 - Attachment is obsolete: true
Attachment #382951 - Flags: superreview?(stejohns)
Attachment #382951 - Flags: review?(lhansen)

Comment 7

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

Comment 8

9 years ago
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?
Attachment #382951 - Flags: superreview?(stejohns)
(Reporter)

Comment 9

9 years ago
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.

Comment 10

9 years ago
then might I suggest:

System.collect()
System.queueCollection()

(you guys decide, just an idea to make the api cleaner)
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> then might I suggest:
> 
> System.collect()
> System.queueCollection()

+1
(Reporter)

Comment 12

9 years ago
Return values? Both GC methods are void, so what would you like to see?
(Assignee)

Comment 13

9 years ago
how about "void"?

Comment 14

9 years ago
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.

Comment 15

9 years ago
On reflection I think the better name and signature for the function are

void System.forceFullCollection()
(Reporter)

Comment 16

9 years ago
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.
Attachment #382951 - Attachment is obsolete: true
Attachment #383272 - Flags: superreview?(stejohns)
Attachment #383272 - Flags: review?(lhansen)
(Reporter)

Comment 17

9 years ago
Created attachment 383273 [details] [diff] [review]
System.forceFullCollection() and System.queueCollection()

Forgot to rename collect() to forceFullCollection().
Attachment #383272 - Attachment is obsolete: true
Attachment #383273 - Flags: superreview?(stejohns)
Attachment #383273 - Flags: review?(lhansen)
Attachment #383272 - Flags: superreview?(stejohns)
Attachment #383272 - Flags: review?(lhansen)

Updated

9 years ago
Attachment #383273 - Flags: review?(lhansen) → review+
(Assignee)

Updated

9 years ago
Attachment #383273 - Flags: superreview?(stejohns) → superreview+
(Reporter)

Comment 18

9 years ago
Patch pushed as changeset 2026:f411eef033d7.

Moving on...
(Reporter)

Updated

9 years ago
Depends on: 502570
(Reporter)

Comment 19

9 years ago
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.
Attachment #387632 - Flags: superreview?(stejohns)
Attachment #387632 - Flags: review?(edwsmith)
(Reporter)

Comment 20

9 years ago
Created attachment 387633 [details]
Zipped performance tests

This set of tests go into performance/language/string. Please change the reviewer chain to your liking.
Attachment #387633 - Flags: superreview?(stejohns)
Attachment #387633 - Flags: review?(edwsmith)

Comment 21

9 years ago
Sandbox unexpected pass is expected. 

The failure was fixed in 2099:9220066e5d01 and then the "expectedfail" was removed in rev 2101:9d6892fe7ce6

Updated

9 years ago
Attachment #387632 - Flags: review?(edwsmith)

Updated

9 years ago
Attachment #387632 - Flags: superreview?(stejohns)
Attachment #387632 - Flags: superreview?(edwsmith)
Attachment #387632 - Flags: review?(stejohns)
(Assignee)

Comment 22

9 years ago
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+
(Assignee)

Updated

9 years ago
Attachment #387633 - Flags: superreview?(stejohns) → superreview+
(Reporter)

Comment 23

9 years ago
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)

Updated

9 years ago
Attachment #387633 - Flags: review?(edwsmith) → review?(dschaffe)

Updated

9 years ago
Attachment #387633 - Flags: review?(dschaffe) → review+

Comment 24

9 years ago
Should include a set of tests that are typed, all other performance/language/string tests have both typed and untyped versions.

Updated

9 years ago
Attachment #387632 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Comment 25

9 years ago
pushed to redux as changeset:   2119:a24fd0ef7bc4
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

9 years ago
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 → ---

Updated

9 years ago
Attachment #388981 - Flags: review?(edwsmith) → review+

Comment 27

9 years ago
Comment on attachment 388981 [details] [diff] [review]
Patch for bugs

nothing obviously wrong
(Assignee)

Comment 28

9 years ago
pushed as changeset:   2150:8207c82df001
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 29

9 years ago
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)

Comment 30

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

Comment 31

9 years ago
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+
(Reporter)

Comment 32

9 years ago
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.
(Reporter)

Comment 33

9 years ago
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.
Assignee: stejohns → daumling
Status: RESOLVED → REOPENED
Attachment #389455 - Flags: superreview?(edwsmith)
Attachment #389455 - Flags: review?(lhansen)
Resolution: FIXED → ---

Comment 34

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

Comment 35

9 years ago
(Oh what nasty formatting.  Trying again:)

@@ -91,17 +92,17 @@
  ...
- if (dataP->abcPtr < _abcStringStart || dataP->abcPtr >= _abcStringEnd)
+ if ((uint32_t) (dataP->abcPtr - _abcStringStart) < _abcStringSize)
(Reporter)

Comment 36

9 years ago
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.
Attachment #389455 - Attachment is obsolete: true
Attachment #389473 - Flags: superreview?(edwsmith)
Attachment #389473 - Flags: review?(lhansen)
Attachment #389455 - Flags: superreview?(edwsmith)

Updated

9 years ago
Attachment #389473 - Flags: review?(lhansen) → review+
(Assignee)

Comment 37

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

Comment 38

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

Comment 39

9 years ago
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?
(Assignee)

Comment 40

9 years ago
(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.
(Reporter)

Comment 41

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

Comment 42

9 years ago
(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.)
(Reporter)

Comment 43

9 years ago
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 44

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

Comment 45

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

Comment 46

9 years ago
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.
(Reporter)

Comment 47

9 years ago
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?
Attachment #389473 - Attachment is obsolete: true
Attachment #392240 - Flags: review?(stejohns)
Attachment #389473 - Flags: superreview?(edwsmith)
(Assignee)

Comment 48

9 years ago
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?
(Reporter)

Comment 49

9 years ago
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.
Attachment #392240 - Flags: superreview?(edwsmith)
(Assignee)

Comment 50

9 years ago
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)?
(Reporter)

Comment 51

9 years ago
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?
(Assignee)

Comment 52

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

Comment 53

9 years ago
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 #389148 - Attachment is obsolete: true
Attachment #392240 - Attachment is obsolete: true
Attachment #393618 - Flags: superreview?(edwsmith)
Attachment #393618 - Flags: review?
Attachment #392240 - Flags: superreview?(edwsmith)
Attachment #392240 - Flags: review?(stejohns)
(Assignee)

Updated

9 years ago
Attachment #393618 - Flags: review? → review?(daumling)
(Reporter)

Comment 54

9 years ago
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+
(Assignee)

Comment 55

9 years ago
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
Attachment #393964 - Flags: superreview?(edwsmith)
Attachment #393964 - Flags: review?(daumling)
(Assignee)

Comment 56

9 years ago
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.
Attachment #393964 - Flags: superreview?(edwsmith)
Attachment #393964 - Flags: review?(daumling)
(Assignee)

Comment 57

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

Comment 58

9 years ago
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)
(Reporter)

Updated

9 years ago
Attachment #396008 - Flags: superreview?(daumling) → superreview+
(Reporter)

Comment 59

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

Comment 60

9 years ago
(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?
(Reporter)

Comment 61

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

Comment 62

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

Comment 63

9 years ago
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.
Attachment #396008 - Attachment is obsolete: true
Attachment #396008 - Flags: review?(edwsmith)
(Assignee)

Comment 64

9 years ago
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.
Attachment #397115 - Flags: superreview?(daumling)
Attachment #397115 - Flags: review?(edwsmith)
(Assignee)

Comment 65

9 years ago
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.
Attachment #397138 - Flags: superreview?(daumling)
Attachment #397138 - Flags: review?(edwsmith)

Updated

9 years ago
Attachment #397115 - Flags: review?(edwsmith) → review+

Updated

9 years ago
Attachment #397138 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 66

9 years ago
Comment on attachment 397115 [details] [diff] [review]
Revise dependent strings to use pointer-plus-offset

pushed as part of changeset:   2423:de974974e963
Attachment #397115 - Attachment is obsolete: true
Attachment #397115 - Flags: superreview?(daumling)
(Assignee)

Comment 67

9 years ago
Comment on attachment 397138 [details] [diff] [review]
Enable static-abc-strings

pushed as part of changeset:   2423:de974974e963
Attachment #397138 - Attachment is obsolete: true
Attachment #397138 - Flags: superreview?(daumling)
(Assignee)

Comment 68

9 years ago
I think one is finally nailed. Closing.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 69

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.