Issue 4596 in pharo: WeakOrderedColllection array is replaced by an Array instead of a WeakArray

Comment #20 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

I really want to release 1.3...So I will just add it and we will see.

There is #growSize, too, which is now not used.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: Closed

Comment #21 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

in 13294/13295


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #22 on issue 4596 by [hidden email]: WeakOrderedColllection array  
is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

#grow is essentially the same as #growAtLast (except keeping open slots at  
start), so I presume Nicolas replaced calls to #grow with ones to  
#growAtLast.

This really should be coupled with a change that makes reset use 1 as  
firstIndex though, as you're otherwise likely to have 1/3rd of initial  
slots unused for the common case of only adding/removing from the end.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #23 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

We should not do changes like these in 1.3... it was a mistake to add this  
change.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #24 on issue 4596 by [hidden email]: WeakOrderedColllection array  
is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Well, the simplest change for 1.3, would be to just change Array reference  
in #grow to  a self arrayType call.



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: Started

Comment #25 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #26 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Agreed with you marcus.

Stef


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #27 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

I will undo the change and instead do the change to #grow to use the  
arrayType.
(Will do that this afternoon).




_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: -Milestone-1.3

Comment #28 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

So I fixed 1.3 with just using #arrayType in #grow. No API change, no  
behavior change in OrderedCollection at all.

I remove the 1.3 tag from this entry, for 1.4 than we can see what the best  
fix is.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #29 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

I agree with Marcus, that's the best decision for 1.3.
For 1.4, I would converge with Squeak becasue Levente optimized the growing  
cases (for some cases), and Pharo already picked some code


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #30 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

nicolas if you see what we should integrate let us know because this is not  
that clear to me.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #31 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

I think the last difference is OrderedCollection>>#reset
Levente optimized it for addAll: / addLast: by setting firstIndex to 1  
while the version in Pharo still set the firstIndex to array size // 3.

For example ((1 to: 100) as: OrderedCollection) triggers a makeRoomAtLast  
in Pharo, not in Squeak.


[| oc |
oc := OrderedCollection new: 10.
1 to: 10 do: [:i | oc addLast: i]] bench.
Squeak4.3 -> '1,400,000 per second.'
Pharo1.4 before this patch -> '734,000 per second.'
Pharo1.4 after this patch -> '1,010,000 per second.'
Pharo1.4 after + resestTo: 1 ->  '1,340,000 per second.'


Of course, Pharo seems a bit more optimized if you addFirst:, but no so  
optimized, only one third of array is available for such use. Note that  
Squeak will move only 1 element when making room at first, which does not  
cost that much, while Pharo will have to move one third of the array...

[| oc |
oc := OrderedCollection new: 10.
1 to: 10 do: [:i | oc addFirst: i]] bench.
Squeak4.3 -> '1,500,000 per second.'
Pharo1.4 before this patch -> '1,120,000 per second.'
Pharo1.4 after this patch -> '713,000 per second.'
Pharo1.4 after + resestTo: 1 -> '1,480,000 per second.'

If you mix addFirst: and addLast: in code,I don't know which of the two  
flavours is worse... Maybe a little advantage to Pharo. And I don't know  
how often mixed adding is used...

[| oc |
oc := OrderedCollection new: 10.
1 to: 10 do: [:i | i even
        ifTrue: [oc addLast: i]
        ifFalse: [oc addFirst: i]]] bench.
Squeak4.3 -> '809,000 per second.'
Pharo1.4 before this patch -> '246,000 per second.'
Pharo1.4 after this patch -> '613,000 per second.'
Pharo1.4 after + resestTo: 1 ->  '807,000 per second.'

Conclusion: these micro benchmarkas are dumb, but yes, we should apply this  
path from Levente AND apply the change of reset (should call self resetTo:  
1)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #32 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

guys, can we please separate the topic(s). Because i think
fixing and issue is one thing, but improving/making it work faster is  
another one.
We could integrate same fixes as in 1.3 to 1.4
and then open another issue, which will put a more optimal version on the  
table.
Because keeping posting into same issue again and again frightens me a lot  
(ouch.. its still not fixed!!) not saying about rest of people :)




_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #33 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

I think that optimizing addLast: is better since this is what add: is  
calling so people may often use it.
But indeed getting all of them faster is better :)

Yes mr Igor. You are right.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #34 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Issue 1686 already is about optimizing, so let's transfer optimization  
issue to it.
Is there any other reason why this issue is still not closed ?



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixToInclude

Comment #35 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Yes, this is not yet in 1.4 (I only reverted 1.3)

I will do that this weekend.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: Closed

Comment #36 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Ok, 13134 and 13135 revert the state to the same as in 1.3 (grow uses  
arrayType, makeRoomAt* use grow).

For perfomance enhancements, we should open a new tracker entry.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #37 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Then I have to re-open Issue 2432 and the changes should be re-applied  
immediately.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixToInclude

Comment #38 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

so I missunderstood...

I re-open this and undo the undo..


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #39 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

I will not interfere and let you continue because I'm lost.



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker