505460 - preallocating reserved sots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently in a few places we duplicate a code pattern to allocate reserved slots for Function and Call objects. It would be nice to have a common function for this code.

Initially I planned to do this within a patch for bug 495061 but that increased patch complexity there.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch adds a new function, js_EnsureReservedSlots, that ensures that reserved slots are allocated. To use it with Function objects I have added js_CountScriptFunctionReserveSlots. That is not a best name but at least it reflects that the function can only be applied to interpreted functions.

Comment 2

[Brendan Eich [:brendan]]

Looks like this will collide with a patch Andreas is working on -- Andreas, please take a look. Maybe minor, just js_AllocSlots -> AllocSlots.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 389988 [details] [diff] [review]
v1

>+        if (!js_EnsureReservedSlots(cx, clone,
>+                                    js_CountScriptFunctionReserveSlots(fun))) {

Really want fun->countReservedSlots().

(Note the 'd' in 'Reserved').

r=me with that, modulo conflict avoidance with [e-mail]'s pending patch.

/be

Comment 4

[Igor Bukanov]

Attachment: v2

The updated patch adds JSFunction::countInterpretedReserveSlots(). I have the explicit "Intepreted" in the name to indicate that the method can only be applied for interpreted functions.

Comment 5

[Igor Bukanov]

To :gal - with which patch this could conflict?

Comment 6

[Andreas Gal :gal]

It collides with the GC-ed scope patch and the inline dslots patch, but thats all easy to fix. I wouldn't worry about it. I can easily rebase.

Comment 7

[Brendan Eich [:brendan]]

No 'd' in countInterpretedReserveSlots, as requested :-(.

The Interpreted name part still seems fruitless to me. The method could return 0 for natives and assert in DEBUG builds that the |this| function is interpreted. Or we could subclass interpreted vs. native functions and put this method only in the former class, but that's for later.

/be

Comment 8

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/ada08e63ab62 - I have landed before I spotted that bad spelling in method's name.

(In reply to comment #7)
> The Interpreted name part still seems fruitless to me. The method could return
> 0 for natives and assert in DEBUG builds that the |this| function is
> interpreted.

Returning 0 for native functions would add a useless if-check.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> http://hg.mozilla.org/tracemonkey/rev/ada08e63ab62 - I have landed before I
> spotted that bad spelling in method's name.
> 
> (In reply to comment #7)
> > The Interpreted name part still seems fruitless to me. The method could return
> > 0 for natives and assert in DEBUG builds that the |this| function is
> > interpreted.
> 
> Returning 0 for native functions would add a useless if-check.

Ok, then just assert. We do similar things elsewhere. It's not as if the name does anything at compile or runtime to prevent a call on a native function.

/be

Comment 10

[Brendan Eich [:brendan]]

Anyway, rs=me on s/Reserve/Reserved/ in the function name.

/be

Comment 11

[Igor Bukanov]

misspellings fix - http://hg.mozilla.org/tracemonkey/rev/36fab1f75e34

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ada08e63ab62