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

Already on GitHub? Sign in to your account

Fix memory leak and possible performance hit #345

Merged
merged 1 commit into from Feb 5, 2013

Conversation

Projects
None yet
2 participants
Contributor

kraz commented Jan 25, 2013

Loading code on demand is not hitting the query cache due to an extra parameter which the ajax method appends:

ss.load.code('/test', function() {
    require('/cached.js');
});

calls: /_serve/code?test&_=1359143112452 (ajax cache: false)

This causes the queryCache to be filled with output data for every subsequent call (on every client refresh, or new request).

This way the browser keeps fresh while the server is not minifying the content on every subsequent request to single client code.

This reminds me that may be better handling of the pack assets procedure could be:

ss.client.options.packAssets = process.env['SS_PACK'] || false;

// Minimize and pack assets if you type: SS_ENV=production node app.js
if ((process.env.SS_ENV || process.env.NODE_ENV || 'production') === 'production' || ss.client.options.packAssets) {
    ss.client.options.packAssets = true;
    ss.client.packAssets({keepOldFiles: true});
}

owenb commented Jan 28, 2013

Hey Krasen

I've not been able to replicate this because the caching is handled on the client. So even though each new request to the server would return new content, if you call

ss.load.code('/test', function() {
    require('/cached.js');
});

multiple times it should never call the server more than once. Are you seeing something different?

I'm going to change the syntax for packing assets in 0.4. It feels really messy in 0.3, mostly because I changed the behaviour late in the day to support pre-processing and caching of asset files.

Cheers,

Owen

Contributor

kraz commented Jan 28, 2013

May be i can't explain it good - this is why i wrote a quick app to illustrate the issue:
https://github.com/kraz/ss-pullreq-345

Long story short:

The caching is indeed handled on the client - calling ss.load.code is working as expected (queries the server only at first request),

on every client refresh, or new request

But imagine 100 different users keep refreshing the page. Every first call to ss.load.code will force the server to stuff an other (duplicated) value in the queryCache array but with different key because calling ss.load.code calls internally $.ajax with cache:false which causes a random &_=1359143112452 to be appended on every request.

The if options.packAssets && queryCache[request.url] is never true. Because options.packAssets is always undefined and even if we set it to true in app.js the 'request.url` is always different because of the above ajax call method.

The issue is not that bad if the app is small with relatively small audience, but i have used to always test with the worst scenarios possible. Just imagine this with 100+ users and application loading (huge) code on demand depending on user pre configured request. Of course its sort of design issue also but still i see flaws in code base.

owenb commented Feb 5, 2013

Thanks Krasen

I see what you mean now. I'll merge this in and look though it again to make sure I can't see any obvious reason why this won't work out as expected. If all is well, it will be included in 0.3.3 which I'm going to release very soon.

Thanks again,

Owen

@owenb owenb pushed a commit that referenced this pull request Feb 5, 2013

Owen Barnes Merge pull request #345 from kraz/master
Fix memory leak and possible performance hit
7c860f9

@owenb owenb merged commit 7c860f9 into socketstream:master Feb 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment