Seaside2.7a1-avi.10

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Seaside2.7a1-avi.10

Avi  Bryant
Note: this message is only relevant for people using Seaside on  
Squeak.  I think.

I've just committed a version to the 2.7a branch which includes a  
couple of changes we use internally for Dabble DB.  The main change I  
just made recently, and although it seems fine, it's hackish enough  
that I'd be curious to have others take a look at it.  It takes a bit  
of explanation.

The background is this: we had some users complaining that if they  
opened up a number of tabs in their browser, sometimes when they went  
back to an old tab the links they clicked on would have no effect.  
This is, of course, because we expire old continuations, and their  
old tabs were falling out of the LRU cache.

So I tried bumping up (way up, to like 500) the number of  
continuations kept in the cache.  Not that surprisingly, I saw a  
massive increase (cripplingly so) in the size of the Seaside  
sessions.  The interesting thing was what a space analysis of these  
huge images showed.  The space wasn't, largely, being used by stored  
method contexts or snapshotted state.  It was almost all instances of  
String, and some random sampling of these showed that almost all of  
them were from CSS class or id attributes.  Huh?

What was happening was this:  I would have a render method that would  
construct a CSS class or ID, usually by combining some string literal  
with some number.  Then I would pass that class or ID into another  
render method.  Minimally, it might look like this:

renderCellForColumn: aNumber on: html
    |class|
    class := 'tableColumn', (aNumber \\ 2 + 1) asString.
    self renderCellWithClass: class on: html

Then that other render method would generate a callback, like this:

renderCellWithClass: aString on: html
   html tableData class: aString; with:
      [html anchor callback: [self doStuff]; text: 'Do stuff']

Because aString was constructed, it was a new instance every time.  
And because it was in the home context for the callback block (even  
though it wasn't used from the block), it was held onto by the  
callback, which is held onto for (in my case) quite a long time.  And  
when you have tables with thousands of cells, each with one of these  
CSS classes, it builds up.

So, ok, one lesson here is to be more careful about constructing  
strings (I could have 'tableColumn1' and 'tableColumn2' literals and  
an if statement), or to intern them.  But I was curious if I could  
solve the problem more generally than that.  After all, the block  
doesn't *need* to be holding onto aString.

So I added a #tempVarRefs method to BlockContext.  This scans the  
bytecodes for the block and returns the indices of any temps that the  
block uses.  I think added a variation of #fixTemps named  
#fixCallbackTemps which, after copying the home context, goes through  
the temps and nils out any that don't appear in #tempVarRefs.

I then changed every reference to #fixTemps within Seaside to  
#fixCallbackTemps.  The upshot is that all stored callbacks in  
Seaside should avoid holding onto any temps that they don't actually  
use.

I'm hoping this will lead to a noticeable reduction in memory  
footprint in the general case, but I haven't done any tests yet.  I  
also haven't done any thorough testing of #tempVarRefs to make sure  
that #fixCallbackTemps does, in fact, leave the semantics of the  
block unchanged compared to #fixTemps.  I would love it if someone  
had the time to review the code and run some such tests.

Meanwhile, we're using it in production with no ill effects reported  
so far...

Cheers,
Avi

_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

RE: Seaside2.7a1-avi.10

Ramon Leon-5
> I'm hoping this will lead to a noticeable reduction in memory
> footprint in the general case, but I haven't done any tests
> yet.  I also haven't done any thorough testing of
> #tempVarRefs to make sure that #fixCallbackTemps does, in
> fact, leave the semantics of the block unchanged compared to
> #fixTemps.  I would love it if someone had the time to review
> the code and run some such tests.
>
> Meanwhile, we're using it in production with no ill effects
> reported so far...
>
> Cheers,
> Avi


I just loaded it, hit one snag with Scriptalicious..

SUAjax>>callback: aBlock
        callback := aBlock fixTemps

Gave a dnu, changed to

SUAjax>>callback: aBlock
        callback := aBlock fixCallbackTemps

And everything seems to work fine, though WATableReport needs ported to
canvas api, so the memory link on the toolbar works.  I fixed mine local,
but have other changes so I can't upload my version.  String still seems
high though...

ByteString 5108 2059088
MethodContext 2468 227056
BlockContext 2389 219788
Array 2709 163104
Association 8651 103812
Dictionary 1683 26928

2 megs of string... Wow.

_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

Re: Seaside2.7a1-avi.10

Philippe Marschall
2006/9/22, Ramon Leon <[hidden email]>:

> > I'm hoping this will lead to a noticeable reduction in memory
> > footprint in the general case, but I haven't done any tests
> > yet.  I also haven't done any thorough testing of
> > #tempVarRefs to make sure that #fixCallbackTemps does, in
> > fact, leave the semantics of the block unchanged compared to
> > #fixTemps.  I would love it if someone had the time to review
> > the code and run some such tests.
> >
> > Meanwhile, we're using it in production with no ill effects
> > reported so far...
> >
> > Cheers,
> > Avi
>
>
> I just loaded it, hit one snag with Scriptalicious..
>
> SUAjax>>callback: aBlock
>         callback := aBlock fixTemps
>
> Gave a dnu, changed to
>
> SUAjax>>callback: aBlock
>         callback := aBlock fixCallbackTemps
>
> And everything seems to work fine, though WATableReport needs ported to
> canvas api, so the memory link on the toolbar works.

There is WACanvasTableReport. We can just push up the methods and then
remove it.

Philippe
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

Re: Seaside2.7a1-avi.10

Philippe Marschall
In reply to this post by Avi Bryant
2006/9/22, Avi Bryant <[hidden email]>:

> Note: this message is only relevant for people using Seaside on
> Squeak.  I think.
>
> I've just committed a version to the 2.7a branch which includes a
> couple of changes we use internally for Dabble DB.  The main change I
> just made recently, and although it seems fine, it's hackish enough
> that I'd be curious to have others take a look at it.  It takes a bit
> of explanation.
>
> The background is this: we had some users complaining that if they
> opened up a number of tabs in their browser, sometimes when they went
> back to an old tab the links they clicked on would have no effect.
> This is, of course, because we expire old continuations, and their
> old tabs were falling out of the LRU cache.
>
> So I tried bumping up (way up, to like 500) the number of
> continuations kept in the cache.  Not that surprisingly, I saw a
> massive increase (cripplingly so) in the size of the Seaside
> sessions.  The interesting thing was what a space analysis of these
> huge images showed.  The space wasn't, largely, being used by stored
> method contexts or snapshotted state.  It was almost all instances of
> String, and some random sampling of these showed that almost all of
> them were from CSS class or id attributes.  Huh?
>
> What was happening was this:  I would have a render method that would
> construct a CSS class or ID, usually by combining some string literal
> with some number.  Then I would pass that class or ID into another
> render method.  Minimally, it might look like this:
>
> renderCellForColumn: aNumber on: html
>     |class|
>     class := 'tableColumn', (aNumber \\ 2 + 1) asString.
>     self renderCellWithClass: class on: html
>
> Then that other render method would generate a callback, like this:
>
> renderCellWithClass: aString on: html
>    html tableData class: aString; with:
>       [html anchor callback: [self doStuff]; text: 'Do stuff']
>
> Because aString was constructed, it was a new instance every time.
> And because it was in the home context for the callback block (even
> though it wasn't used from the block), it was held onto by the
> callback, which is held onto for (in my case) quite a long time.  And
> when you have tables with thousands of cells, each with one of these
> CSS classes, it builds up.
>
> So, ok, one lesson here is to be more careful about constructing
> strings (I could have 'tableColumn1' and 'tableColumn2' literals and
> an if statement), or to intern them.  But I was curious if I could
> solve the problem more generally than that.  After all, the block
> doesn't *need* to be holding onto aString.
>
> So I added a #tempVarRefs method to BlockContext.  This scans the
> bytecodes for the block and returns the indices of any temps that the
> block uses.  I think added a variation of #fixTemps named
> #fixCallbackTemps which, after copying the home context, goes through
> the temps and nils out any that don't appear in #tempVarRefs.
>
> I then changed every reference to #fixTemps within Seaside to
> #fixCallbackTemps.  The upshot is that all stored callbacks in
> Seaside should avoid holding onto any temps that they don't actually
> use.
>
> I'm hoping this will lead to a noticeable reduction in memory
> footprint in the general case, but I haven't done any tests yet.  I
> also haven't done any thorough testing of #tempVarRefs to make sure
> that #fixCallbackTemps does, in fact, leave the semantics of the
> block unchanged compared to #fixTemps.  I would love it if someone
> had the time to review the code and run some such tests.
>
> Meanwhile, we're using it in production with no ill effects reported
> so far...

It's great to have you back doing Seaside development.

I can not help but think if we had a VM that ....

Philippe
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

Re: Seaside2.7a1-avi.10

Avi  Bryant

On Sep 22, 2006, at 2:11 PM, Philippe Marschall wrote:

> It's great to have you back doing Seaside development.

Personally, I think it's great that others are carrying things  
forward despite my neglect :).

> I can not help but think if we had a VM that ....

Can you elaborate on this?

Avi
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

Re: Seaside2.7a1-avi.10

Philippe Marschall
2006/9/22, Avi Bryant <[hidden email]>:

>
> On Sep 22, 2006, at 2:11 PM, Philippe Marschall wrote:
>
> > It's great to have you back doing Seaside development.
>
> Personally, I think it's great that others are carrying things
> forward despite my neglect :).
>
> > I can not help but think if we had a VM that ....
>
> Can you elaborate on this?

With CleanBlocks this wouldn't be a problem, wouldn't it?

Philippe
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

Re: Seaside2.7a1-avi.10

Jason Johnson-3
In reply to this post by Avi Bryant
Avi Bryant wrote:

> Note: this message is only relevant for people using Seaside on
> Squeak.  I think.
>
> I've just committed a version to the 2.7a branch which includes a
> couple of changes we use internally for Dabble DB.  The main change I
> just made recently, and although it seems fine, it's hackish enough
> that I'd be curious to have others take a look at it.  It takes a bit
> of explanation.
>
> <snip>

I noticed in the repository you doing some testing on reducing the
number of continuations.  Is this still being looked at?  Did you hit a
point that proved it wasn't going to work?  Or maybe it's already rolled
in and I just didn't understand how Monticello works. :)
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

Re: Seaside2.7a1-avi.10

Avi  Bryant

On Sep 24, 2006, at 2:55 AM, Jason Johnson wrote:

> Avi Bryant wrote:
>> Note: this message is only relevant for people using Seaside on  
>> Squeak.  I think.
>>
>> I've just committed a version to the 2.7a branch which includes a  
>> couple of changes we use internally for Dabble DB.  The main  
>> change I just made recently, and although it seems fine, it's  
>> hackish enough that I'd be curious to have others take a look at  
>> it.  It takes a bit of explanation.
>>
>> <snip>
>
> I noticed in the repository you doing some testing on reducing the  
> number of continuations.  Is this still being looked at?  Did you  
> hit a point that proved it wasn't going to work?  Or maybe it's  
> already rolled in and I just didn't understand how Monticello  
> works. :)

I'm still interested in doing this, and there's no reason it won't  
work, I just haven't had the opportunity to roll up my sleeves and  
get back into it yet.  If anyone else is interested in this, I'd be  
happy to chat about what needs doing (once I look at the code and  
remind myself :).

Avi
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

fixCallbackTemps

Michel Bany
In reply to this post by Avi Bryant
> So I added a #tempVarRefs method to BlockContext.  This scans the  
> bytecodes for the block and returns the indices of any temps that  
> the block uses.  I think added a variation of #fixTemps named  
> #fixCallbackTemps which, after copying the home context, goes  
> through the temps and nils out any that don't appear in #tempVarRefs.
>
> I then changed every reference to #fixTemps within Seaside to  
> #fixCallbackTemps.  The upshot is that all stored callbacks in  
> Seaside should avoid holding onto any temps that they don't  
> actually use.
>

Avi,

I can see many references to #fixTemps in the live callback methods
of the SeasideAsync package.
Should all of these be replaced with #fixCallbackTemps as well ?

A second question.
You did not replace #fixTemps in #clearHandlers and  
#unregisterExpiredHandlers.
Was it intentional ?

Thanks,
Michel.


_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside
Reply | Threaded
Open this post in threaded view
|

Re: fixCallbackTemps

Avi  Bryant

On Oct 30, 2006, at 5:32 AM, Michel Bany wrote:

>> So I added a #tempVarRefs method to BlockContext.  This scans the  
>> bytecodes for the block and returns the indices of any temps that  
>> the block uses.  I think added a variation of #fixTemps named  
>> #fixCallbackTemps which, after copying the home context, goes  
>> through the temps and nils out any that don't appear in #tempVarRefs.
>>
>> I then changed every reference to #fixTemps within Seaside to  
>> #fixCallbackTemps.  The upshot is that all stored callbacks in  
>> Seaside should avoid holding onto any temps that they don't  
>> actually use.
>>
>
> Avi,
>
> I can see many references to #fixTemps in the live callback methods
> of the SeasideAsync package.
> Should all of these be replaced with #fixCallbackTemps as well ?

Probably.  There at least shouldn't be any harm to doing so.

> A second question.
> You did not replace #fixTemps in #clearHandlers and  
> #unregisterExpiredHandlers.
> Was it intentional ?

Yes, because I know it won't help in those cases.

Avi
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside