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

Add cache option to improve serialization performance #19

Merged
merged 10 commits into from
Jan 23, 2014
Merged

Add cache option to improve serialization performance #19

merged 10 commits into from
Jan 23, 2014

Conversation

ericf
Copy link
Collaborator

@ericf ericf commented Jan 21, 2014

It's common for exposed values to not change during the lifetime of an app or request once they've been passed to the app.expose() or res.expose() methods. When this is the case, the cache option can be set which signals Express State that it's safe to eagerly serialize the value, making repeated calls to Exposed#toString() much more efficient.

The most common use case for the cache option is for app-level data which doesn't change, like configuration:

app.expose(config, 'app.config', {cache: true});

Benchmarks

The follow are the benchmarks comparing master with this branch.

master

simpleObj:
JSON.stringify( simpleObj ) x 595,990 ops/sec ±3.09% (91 runs sampled)
serialize( simpleObj ) x 280,078 ops/sec ±5.14% (83 runs sampled)
simple.toString() x 233,066 ops/sec ±5.61% (79 runs sampled)

PNM:
pnmApp.toString() x 18,257 ops/sec ±4.58% (86 runs sampled)
pnmRes.toString() x 1,747 ops/sec ±2.71% (91 runs sampled)

YT:
ytApp.toString() x 9,497 ops/sec ±6.10% (83 runs sampled)
ytRes.toString() x 188 ops/sec ±5.44% (76 runs sampled)

cache

simpleObj:
JSON.stringify( simpleObj ) x 526,089 ops/sec ±4.64% (82 runs sampled)
serialize( simpleObj ) x 290,813 ops/sec ±3.85% (88 runs sampled)
simple.toString() x 220,056 ops/sec ±3.56% (84 runs sampled)
simpleCached.toString() x 1,972,394 ops/sec ±4.58% (85 runs sampled)

PNM:
pnmApp.toString() x 15,840 ops/sec ±5.44% (84 runs sampled)
pnmAppCached.toString() x 241,270 ops/sec ±4.92% (83 runs sampled)
pnmRes.toString() x 1,427 ops/sec ±5.02% (79 runs sampled)
pnmResAppCached.toString() x 1,737 ops/sec ±3.41% (90 runs sampled)
pnmResCached.toString() x 42,577 ops/sec ±3.44% (88 runs sampled)

YT:
ytApp.toString() x 9,509 ops/sec ±6.08% (85 runs sampled)
ytAppCached.toString() x 339,479 ops/sec ±5.94% (71 runs sampled)
ytRes.toString() x 182 ops/sec ±4.99% (75 runs sampled)
ytResAppCached.toString() x 194 ops/sec ±4.22% (78 runs sampled)
ytResCached.toString() x 29,270 ops/sec ±4.65% (88 runs sampled)

Todos

  • Update docs for cached option
  • Update HISTORY.md
  • Include note about deprecated expose( obj [, namespace [, local]] ) API

ericf added 4 commits January 15, 2014 13:58
It's common for data being exposed to not change between requests (this
is most common for app-level data); this change adds support for a
`cache` option which signal express-state that it's safe to cache and
reused the serialized form of the data.

    app.expose(constConfig, 'config', {cache: true});
This fixes an issue with namespaces to make the following work as
expected:

    app.expose('foo', 'window.foo');
    console.log(res.locals.state); // => window.foo = "foo";

    app.expose('bar', 'window.bar');
    console.log(res.locals.state); // => window.foo = "foo";

This also brings test coverage back to 100% and adds more code comments.
@ghost ghost assigned ericf Jan 21, 2014
}, this);

// Stores the new exposed namespace and its current value.
namespaces.push(namespace);
this[namespace] = value;

// When it's deemed safe to cache the serialized form of the `value` becuase
// it won't change, run the serialization process once, egarly. The result

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"egarly" means "eagerly" ?

@ericf
Copy link
Collaborator Author

ericf commented Jan 21, 2014

@drewfish thanks for finding my typos :)

@caridy
Copy link
Contributor

caridy commented Jan 22, 2014

LGTM

});

it('should inherit new namespaces', function () {
it('should inherit new namespaces form super', function () {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: should inherit new namespaces from super

ericf added a commit that referenced this pull request Jan 23, 2014
Add `cache` option to improve serialization performance
@ericf ericf merged commit 2215823 into master Jan 23, 2014
@ericf ericf deleted the cache branch January 25, 2014 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants