763519 - Bring back ByteArray capacity adjustment policy induced by length setter

Status: RESOLVED INCOMPLETE

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Spawned off of Bug 699176, comment 54

The logic in ByteArray::Grower::run reintroduces the doubling behavior

Grower::run was introduced by:

 http://hg.mozilla.org/tamarin-redux/rev/3b1b1c96a04c

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch T v1: regression test for overzealous capacity growth

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v1: fix policy

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch T v2: add second test checking that length reduction 'can' reduce capacity

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 631972 [details] [diff] [review]
patch B v1: fix policy

Krzyzstof: for reference, compare against the version of the ByteArray.cpp code from    rev 7394:c3eb08266f02 aka CL @1063253

Also, if you do not have time for review, please delegate to e.g. wmaddox.  (But probably better to have someone doing concurrency take care of the sanity-checking here.)

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #5)
> Krzyzstof: for reference, compare against the version of the ByteArray.cpp
> code from    rev 7394:c3eb08266f02 aka CL @1063253

In particular, these are the differences I was focusing on when I reviewed the above:

The changes to ByteArray::SetLengthCommon, where newCap was dropped:

 http://hg.mozilla.org/tamarin-redux/diff/3b1b1c96a04c/core/ByteArrayGlue.cpp#l1.324

and The changes to Grower::ReallocBackingStore, where newCapacity was dropped:

 http://hg.mozilla.org/tamarin-redux/diff/3b1b1c96a04c/core/ByteArrayGlue.cpp#l1.188

Comment 7

[Krzysztof Palacz]

Comment on attachment 631972 [details] [diff] [review]
patch B v1: fix policy

Review of attachment 631972 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. The disappearance of the original code might have been a result of an integration, although I haven't done the forensics to verify. Unfortunately this code just changed again in the concurrency branch, and would have to be reintegrated. Not sure if this is a reason to hold up this patch though.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Krzysztof Palacz from comment #7)
> Comment on attachment 631972 [details] [diff] [review]
> patch B v1: fix policy

Landed in AVM branch in CL 1076034.  (Working on integrating to Main branch now.)

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Krzysztof Palacz from comment #7)
> Unfortunately this code just changed again in the concurrency branch, and
> would have to be reintegrated. Not sure if this is a reason to hold up this
> patch though.

Sigh.

I should have paid more attention to this comment; it looks like a member of the concurrency team may have independently put in a different fix for the problem (and then pushed it to Main branch).  I guess I'll review and resolve one-way-or-another.

Comment 10

[Dan Schaffer]

Comment on attachment 631974 [details] [diff] [review]
patch T v2: add second test checking that length reduction 'can' reduce capacity

testcase looks good.  I ran it before and after the fix patch. I confirmed the test used to fail and passes now.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #9)
> I guess I'll review and resolve one-way-or-another.

(the outside changes didn't resolve the problem.  i rebased the patch and will be pushing shortly.)

Comment 12

[Tamarin Bot]

changeset: 7431:48481330bf28
user:      Felix Klock II <[e-mail]>
summary:   Bug 763519: bring back ByteArray capacity adj policy induced by length setter (r=kpalacz).

http://hg.mozilla.org/tamarin-redux/rev/48481330bf28

Comment 13

[Tamarin Bot]

changeset: 7432:31ed638e211b
user:      Felix Klock II <[e-mail]>
summary:   Bug 763519: Backed out CL 1076034 aka changeset 48481330bf28 (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/31ed638e211b

Comment 14

[Tamarin Bot]

changeset: 7433:b93a61ccc3bb
user:      Felix Klock II <[e-mail]>
summary:   Bug 757375: latest round of concurrency changes (p=jasowill, r=kpalacz,bgetlin,dtomack).

http://hg.mozilla.org/tamarin-redux/rev/b93a61ccc3bb

Comment 15

[Tamarin Bot]

changeset: 7436:83028a01b0c6
user:      Felix Klock II <[e-mail]>
summary:   Bug 763519: fix ByteArray growth policy post CL @1076050 (r=kpalacz).

http://hg.mozilla.org/tamarin-redux/rev/83028a01b0c6

Comment 16

[Tamarin Bot]

changeset: 7437:bdb826bd6701
user:      Felix Klock II <[e-mail]>
summary:   Bug 763519: regression test checking new capacity adjustment policy (r=dschaffe).

http://hg.mozilla.org/tamarin-redux/rev/bdb826bd6701

Comment 17

[Brent Baker]

Reopening bug for investigation.

Tom, Felix;
We are seeing this test case fail, runs out of memory and throws Error #1000 under WindowsRT when the interpreter is active.

Comment 18

[Brent Baker]

(In reply to Brent Baker from comment #17)
> Reopening bug for investigation.
> 
> Tom, Felix;
> We are seeing this test case fail, runs out of memory and throws Error #1000
> under WindowsRT when the interpreter is active.

My apologizes, the test fails under ALL execution modes, hybrid, jit and interp.

Comment 19

[Trevor Baker]

Also see WE:3283218 and bz bug 775743

Comment 20

[Tamarin Bot]

changeset: 7498:02cc117784aa
user:      Brent Baker <[e-mail]>
summary:   Bug 763519: mark expected failure under winrt

http://hg.mozilla.org/tamarin-redux/rev/02cc117784aa

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Brent Baker from comment #17)
> Reopening bug for investigation.
> 
> Tom, Felix;
> We are seeing this test case fail, runs out of memory and throws Error #1000
> under WindowsRT when the interpreter is active.

Forked off into Bug 788461.