Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto Reload of a page and memory consumption #611

Closed
GoogleCodeExporter opened this issue Mar 25, 2015 · 18 comments
Closed

Auto Reload of a page and memory consumption #611

GoogleCodeExporter opened this issue Mar 25, 2015 · 18 comments

Comments

@GoogleCodeExporter
Copy link

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

I wanted to program a web-page, which reloads itselfs automatically. To load it 
automaticall I changed the updateRoot: method to:

  super updateRoot: aHTMLRoot.
  aHTMLRoot meta
    responseHeaderName: 'refresh' ;
    content: 1 greaseString

The interval of "1" is not practically - but only for testing. The 
renderContentOn: method is a simple method producing large pages. M Pharo image 
starts with 32 MByte and increases, increases - until Pharo throws an 
exception, that it is out of memory. Therefore I started a Chrome browser and 
made a test within four tabs of this browser.

Perhaps this is only a programming error ?


What version of the product are you using? On what operating system?
 I used the downloadable Seaside-3.0-final.app.zip (Pharo), running under Windows 7 Home/64 Bit.

Original issue reported on code.google.com by Marten.F...@googlemail.com on 13 Nov 2010 at 9:18

Attachments:

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

Original attachment was empty ...

Original comment by Marten.F...@googlemail.com on 13 Nov 2010 at 9:20

  • Added labels: ****
  • Removed labels: ****

Attachments:

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

Original comment by philippe...@gmail.com on 17 Nov 2010 at 9:53

  • Added labels: Platform-Pharo, Version-Seaside3.0
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

There's something not right here, so I'm bumping the priority.

What the code does is it creates a new session with every reload (you'd have to 
use session cookies if you don't want this) with 10000 callbacks. That's going 
to create a lot of big sessions so I set the session timeout to 2 seconds. I'd 
expect the number of sessions to be constant but that's not what's happening. 
After some time I get hundres of sessions although they are all expired.

Looking at the code here's my understanding of what happens. Every ten 
successful session lookups (#retrieved:key:) WARetrievalIntervalReapingStrategy 
reaps the cache. But since we only create sessions in this case and never 
access them we run out of memory. This is quite a departure from our earlier 
behavior of reaping the cache after creating ten sessions, not after accessing 
ten sessions. I believe the earlier behavior is better because it puts the cost 
on session creation and not on session access. To get back to the old behavior 
I think we should set the miss strategy to  WARetrievalIntervalReapingStrategy 
which should implement #missed: instead of #retrieved:key: and set the to a 
NoOpReapingStrategy.

Original comment by philippe...@gmail.com on 5 Dec 2010 at 9:32

  • Changed state: Accepted
  • Added labels: Priority-High
  • Removed labels: Platform-Pharo, Priority-Medium
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

Looking at WACache and WARetrievalIntervalReapingStrategy again it seems that 
changing #retrieved:key: to  #stored:key: would get us back to the old behavior 
without any need for reconfiguration or something.

Original comment by philippe...@gmail.com on 6 Dec 2010 at 6:42

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

Original comment by philippe...@gmail.com on 7 Dec 2010 at 7:06

  • Changed state: Started
  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

With a session timeout of two seconds memory use is stable now.

Name: Seaside-Core-pmm.672
Author: pmm
Time: 7 December 2010, 8:43:46 pm
UUID: a0843a91-edff-44cd-bd0f-5059cc077a99
Ancestors: Seaside-Core-pmm.671

- Issue 611:    Auto Reload of a page and memory consumption
- http://code.google.com/p/seaside/issues/detail?id=611

Original comment by philippe...@gmail.com on 7 Dec 2010 at 7:45

  • Changed state: Fixed
  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

I think this was an intentional change because if nobody is creating new 
sessions, the old sessions never got expired and people preferred that they 
did. What if you implemented both methods?

Of course a periodic reaping strategy using a separate thread would probably be 
a better solution anyway - it's just not so easy to make it platform-agnostic...

Original comment by jfitz...@gmail.com on 7 Dec 2010 at 10:23

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

If you don't create new sessions there's no need for reaping because cache 
doesn't grow.

Original comment by philippe...@gmail.com on 8 Dec 2010 at 5:44

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

I'm not sure that background reaping is a better idea. We'd have to lock the 
cache and eat the CPU. So we'd still block other processes. What would IMHO be 
a better option is to use either a b-tree or a list so we have to traverse only 
the items that are expired instead of every item, see Issue 262.

Original comment by philippe...@gmail.com on 8 Dec 2010 at 6:18

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

I realize there's no *need* for them to be reaped. But people found it 
confusing that their last n session objects hang around forever in the cache. 
It doesn't really bother me, but that's my memory of the discussion that 
prompted the change.

As for the background process, the cache is locked during reaping anyway so I 
don't see a difference there. With an independent process, there's a decent 
chance that the reaping will happen between requests rather than at the 
beginning of one. Isn't the worst case (it happens just as a request comes in) 
the same as now and the best case (it happens when no requests are coming in) 
much better?

Original comment by jfitz...@gmail.com on 8 Dec 2010 at 8:51

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

The problem is that reaping is not free. I'd rather not impose the cost of 
reaping when it is not necessary to everybody so that a few people have a 
different feeling. We can include a second reaping strategy that reaps every 
_n_ accesses but I'm against making it the default.

Original comment by philippe...@gmail.com on 8 Dec 2010 at 9:59

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

I'm not convinced. People who are concerned about the performance should use a 
time-based reaping strategy running externally anyway (or use an expiry policy 
that is LRU and size-based or something). If you are expiring sessions after 
600 seconds, there is little point in reaping on even every session creation: 
no matter how many sessions are created per second, there will be nothing new 
to reap; it would be much more efficient to reap every 5 or 10 seconds. We 
should ideally provide such a strategy, though it may be platform-specific.

On a low-traffic site, you won't waste much effort reaping on every nth access, 
and it gives a better (and cross-platform) approximation of the desired 
time-based expiry.

If nothing else and you're determined that this be the default behaviour, 
having a strategy called RETRIEVALInterval that actually operates only on 
insertion is confusing. We need a new class (or a rename) for the new behaviour.

Original comment by jfitz...@gmail.com on 8 Dec 2010 at 11:45

  • Changed state: Started
  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

OK, let me try a more formal approach.

- Let's assume the number of live sessions is constant. If it isn't then we run 
out of memory any way.
- If the number of live sessions is constant that means the number of new 
sessions created during an interval of time must match the number of sessions 
expired in the same interval. Otherwise the number of live sessions would 
increase.
- That means reaping on session creation is preferable to reaping on cache 
access because on session creation we can expect to reap _n_ sessions, where _n 
- 1_ is the number of session creations we didn't do reaping. On cache access 
we may not even be able to to reap a single session because they are longer 
lived.

As for renaming I agree but that will lead to obsoletes. Do you have an idea 
how we can solve the upgrade issue so people don't have to build a new image 
from scratch?

Original comment by philippe...@gmail.com on 9 Dec 2010 at 6:34

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

You seem to be arguing at cross-purposes. I am not saying that reaping on every 
access is the most efficient solution. I said that (a) a time-based reaping 
seems preferable in terms of performance and behaviour given the default expiry 
policy; (b) on-access reaping is a closer in behaviour to the ideal; (c) the 
difference in performance isn't going to be an issue for the majority of our 
users anyway; and (d) the users who do care about that performance should use 
the ideal solution mentioned in a. (or a different expiry policy).

I would suggest adding a new strategy class and reverting the other one to its 
old behaviour (plus a fix so it reaps on insertion as well as on access, I 
guess). We shouldn't delete it without deprecating it anyway, and I actually 
still think it's a perfectly valid approach. I'd even still consider arguing it 
should remain the default, but I doubt I care enough to bother.

Original comment by jfitz...@gmail.com on 9 Dec 2010 at 12:05

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

I vote to revert to the behavior of Seaside 2.8 (Seaside-Core-pmm.672). The 
current implementation is a major security issue that can be easily exploited. 
We know that the previous behavior generally works well and we don't have the 
manpower to implement the suggested more sophisticated strategies right now.

Original comment by renggli on 9 Dec 2010 at 4:41

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

The decision has *nothing* to do with security. We can just fix the original 
bug by making sure the cache is reaped even on the initial request. Frankly 
even as it was when the bug was reported it was only a DOS opportunity if there 
were no other users of the system making real requests at the same time (in 
other words if there was no Service to Deny in the first place).

If you both want different behaviour, fine, go ahead. I think you're arguing 
for a mechanism that is less convenient than the old one and less performant 
than a good solution, but frankly I just don't care that much.

But I *do* care if we have a class behaving differently than its name implies. 
So please just revert the original class and fix its bug. Then feel free to add 
any other reaping strategy you want in a new class. I don't think we can or 
should delete the existing class right now. If you both want to change the 
default, I again don't care enough to argue about it, so feel free.

Original comment by jfitz...@gmail.com on 9 Dec 2010 at 6:01

  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

Name: Seaside-Core-pmm.673
Author: pmm
Time: 10 December 2010, 6:52:26 am
UUID: ff049222-542d-41b6-9fab-99743eb3db91
Ancestors: Seaside-Core-pmm.672

- make retrieval strategy reap on retrieval and access
- add access only stragety
- make access only stragety the default
- Issue 611:    Auto Reload of a page and memory consumption
- http://code.google.com/p/seaside/issues/detail?id=611

Original comment by philippe...@gmail.com on 10 Dec 2010 at 5:53

  • Changed state: Fixed
  • Added labels: ****
  • Removed labels: ****
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Mar 25, 2015

Issue 575 has been merged into this issue.

Original comment by philippe...@gmail.com on 29 Dec 2010 at 4:55

  • Added labels: ****
  • Removed labels: ****
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.