Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Use a WeakMap to store memoized values #298

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use a WeakMap to store memoized values #298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fca2d93
Use WeakMap to memoize rather than mutating ctx
rtsao Sep 19, 2018
4b0b7b8
Remove vestigial property from memoization test
rtsao Sep 19, 2018
612a16d
Remove browser-specific memoization
rtsao Sep 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions src/memoize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,35 @@ import type {Context} from './types.js';

type MemoizeFn<A> = (ctx: Context) => A;

function Container() {}

export function memoize<A>(fn: MemoizeFn<A>): MemoizeFn<A> {
const memoizeKey = __NODE__ ? Symbol('memoize-key') : new Container();
return function memoized(ctx: Context) {
if (ctx.memoized.has(memoizeKey)) {
return ctx.memoized.get(memoizeKey);
if (__BROWSER__) {
return browserMemoize(fn);
}

const wm = new WeakMap();
Copy link
Contributor

@lhorie lhorie Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We could put this logic be in an else block, and/or do feature detection in browser

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we ship core-js polyfills when necessary, so perhaps we can just assume WeakMap exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Polyfill doesn't GC though, does it?

Copy link
Member Author

@rtsao rtsao Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure but it the core-js polyfill seems to use this weak collection implementation (which I believe should allow for GC): https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/collection-weak.js

return ctx => {
if (wm.has(ctx)) {
return ((wm.get(ctx): any): A); // Refinement doesn't seem to work
} else {
const result = fn(ctx);
wm.set(ctx, result);
return result;
}
};
}

/**
* There is only ever a single ctx object in the browser.
* Therefore we can use a simple memoization function.
Copy link
Member Author

@rtsao rtsao Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not totally sure if this assumption is valid. This is certainly the case in real applications, but I wonder if this may not be true with simulation tests that run in the browser (although I don't think this is a thing).

But if this assumption is not valid, we should add a test case for this.

*/
function browserMemoize<A>(fn: MemoizeFn<A>): MemoizeFn<A> {
let memoized;
let called = false;
return ctx => {
if (!called) {
memoized = fn(ctx);
called = true;
}
const result = fn(ctx);
ctx.memoized.set(memoizeKey, result);
return result;
return memoized;
};
}
1 change: 0 additions & 1 deletion src/plugins/timing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ const timing: TimingPlugin = {
export const TimingToken: Token<TimingPlugin> = createToken('TimingToken');

function middleware(ctx, next) {
ctx.memoized = new Map();
const {start, render, end, downstream, upstream} = timing.from(ctx);
ctx.timing = {
start,
Expand Down