patch for KJS "backwards for declarations" bug

[Maciej Stachowiak]

On Oct 18, 2003, at 4:56 PM, Harri Porten wrote:

> On Sat, 18 Oct 2003, Darin Adler wrote:
>
>> This patch fixes a bug I introduced in JavaScriptCore last December
>> when I changed it to use iteration rather than recursion to process
>> lists. I missed one case where a list needs to be reversed because 
>> it's
>> built backwards.
>
> Applied. Thanks.
>
>> Pages that show this bug are
>> <http://x.isomorphic.com/Safari_Tests/for_loop_jsError.html>, dragging
>
> A mean one.
>
> The earlier suggestion you are referring to: I think we should indeed
> follow that path as the resulting code should both be smaller and
> faster. Will see if I still recall the principle. Is this list archived
> ? :)

This was the suggestion:

> class PropertyValueNode : public Node {
>   public:
>     // list is circular during construction, cut in ObjectLiteralNode 
> ctor
>     PropertyValueNode(PropertyNode *n, Node *a)
>       : name(n), assign(a), list(this) { }
>     PropertyValueNode(PropertyNode *n, Node *a, PropertyValueNode *l)
>       : name(n), assign(a), list(l) { l->list = this; }

But it seems to have a bug, I don't see how this could make anything 
but a two-element circular list, since "this" and "l" will always point 
to each other with their "list" pointers. I think this is what you had 
in mind:

   PropertyValueNode(PropertyNode *n, Node *a, PropertyValueNode *l)
       : name(n), assign(a), list(l->list) { l->list = this; }

I just made some box and pointer diagrams and it looks like this will 
make the right circular list.

>     // ...
> };
>
> class ObjectLiteralNode : public Node {
>   public:
>     // empty literal
>     ObjectLiteralNode() : list(0) { }
>     // l points to last list element, get and detach pointer to first 
> one
>     ObjectLiteralNode(PropertyValueNode *l) : list(l->list) { l->list 
> = 0;}
>     // ...
>   };


In any case, none of our profiles show any time spent in reverseList. 
And I don't think it makes a huge improvement algorithmically, either 
way there are 2*N assignments of the "list" pointer. The suggested 
change does have better locality of reference, though, so it should be 
a win for really huge lists at the expense of slightly trickier code.

Regards,
Maciej