From 41e26aaa79e26c5dfe616fa8b64fe32f27d47a09 Mon Sep 17 00:00:00 2001 From: Bo Borgerson Date: Wed, 5 Apr 2017 16:22:43 -0700 Subject: [PATCH] Front load and cache some page chain class prep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This takes some work that used to happen for each request and caches it. This is mostly groundwork for fine-grained instrumentation of page lifecycle methods. It incidentally also slightly reduces the overhead of building up the page chain object during each request. [core/__bench__/handlePage.js](https://github.com/redfin/react-server/pull/926) before: ``` 1 0.000481308279135711 10 0.0009405891195623999 100 0.005067864883424908 1000 0.04754033069230767 ``` [core/__bench__/handlePage.js](https://github.com/redfin/react-server/pull/926) after: ``` 1 0.0004272630582241284 10 0.0006927164659505115 100 0.0034683026867557273 1000 0.031324801985714294 ``` Basically reduces middleware overhead to from ~50µs to ~30µs. Not a huge deal, but the instrumentation is likely to add a little overhead, so it's nice to pay some of that down ahead of time. :grin: --- .../core/__tests__/reactMiddlewareSpec.js | 6 +- .../react-server/core/context/Navigator.js | 27 +---- packages/react-server/core/util/PageUtil.js | 112 ++++++++++++------ 3 files changed, 81 insertions(+), 64 deletions(-) diff --git a/packages/react-server/core/__tests__/reactMiddlewareSpec.js b/packages/react-server/core/__tests__/reactMiddlewareSpec.js index e0fa36028..0028eb3cf 100644 --- a/packages/react-server/core/__tests__/reactMiddlewareSpec.js +++ b/packages/react-server/core/__tests__/reactMiddlewareSpec.js @@ -13,7 +13,7 @@ describe("renderMiddleware", () => { describe("null values", () => { beforeAll(() => { - page = PageUtil.createPageChain([new NullValuesPage()]); + page = PageUtil.createPageChain(NullValuesPage); }); beforeEach(() => { @@ -51,7 +51,7 @@ describe("renderMiddleware", () => { describe("promises with null values", () => { beforeAll(() => { - page = PageUtil.createPageChain([new NullValuePromisesPage()]); + page = PageUtil.createPageChain(NullValuePromisesPage); }); beforeEach(() => { @@ -89,7 +89,7 @@ describe("renderMiddleware", () => { describe("good values", () => { beforeAll(() => { - page = PageUtil.createPageChain([new NormalValuesPage()]); + page = PageUtil.createPageChain(NormalValuesPage); }); beforeEach(() => { diff --git a/packages/react-server/core/context/Navigator.js b/packages/react-server/core/context/Navigator.js index c3bd80f6c..b842ccb17 100644 --- a/packages/react-server/core/context/Navigator.js +++ b/packages/react-server/core/context/Navigator.js @@ -136,19 +136,7 @@ class Navigator extends EventEmitter { } handlePage(pageConstructor, request, type) { - // instantiate the pages we need to fulfill this request. - var pageClasses = []; - - this._addPageMiddlewareToArray(this._globalMiddleware, pageClasses); - this._addPageMiddlewareToArray([pageConstructor], pageClasses); - - var pages = pageClasses.map((pageClass) => { - if (Object.getOwnPropertyNames(pageClass).length === 0) { - throw new Error("Tried to instantiate a page or middleware class that was an empty object. Did you forget to assign a class to module.exports?"); - } - return new pageClass(); - }); - var page = PageUtil.createPageChain(pages); + var page = PageUtil.createPageChain(pageConstructor, this._globalMiddleware); this.emit("page", page); @@ -200,19 +188,6 @@ class Navigator extends EventEmitter { } - /** - * recursively adds the middleware in the pages array to array. - */ - _addPageMiddlewareToArray(pages, array) { - if (!pages) return; - pages.forEach((page) => { - if (page.middleware) { - this._addPageMiddlewareToArray(page.middleware(), array); - } - array.push(page); - }); - } - getState () { return { loading: this._loading, diff --git a/packages/react-server/core/util/PageUtil.js b/packages/react-server/core/util/PageUtil.js index 513484780..2aa5b8d70 100644 --- a/packages/react-server/core/util/PageUtil.js +++ b/packages/react-server/core/util/PageUtil.js @@ -145,6 +145,15 @@ var PAGE_METHODS = { getResponseData : [() => "", Q], }; +// Apply the standardization functions to each default implementation. +Object.keys(PAGE_METHODS).forEach(method => { + var [defaultImpl, standardize] = PAGE_METHODS[method]; + var standardizedDefaultImpl = function() { + return standardize(defaultImpl.apply(this, arguments)); + } + PAGE_METHODS[method] = [standardizedDefaultImpl, standardize]; +}); + // These are similar to `PAGE_METHODS`, but differ as follows: // // - They are not chained. @@ -216,26 +225,6 @@ function makeSetter(key){ } } -// This attaches `PAGE_MIXIN` methods to page/middleware classes. -// -// It does this only _once_, and thereafter short-circuits. -// -function lazyMixinPageUtilMethods(page){ - var proto = Object.getPrototypeOf(page); - if (proto._haveMixedInPageUtilMethods) return; - - proto._haveMixedInPageUtilMethods = true; - - Object.keys(PAGE_MIXIN).forEach(method => { - if (proto[method]){ - throw new Error(`PAGE_MIXINS method override: ${ - (proto.constructor||{}).name - }.${method}`); - } - proto[method] = PAGE_MIXIN[method]; - }); -} - // These `standardize*` functions show what will happen to the output of your // page methods. // @@ -318,14 +307,6 @@ function logInvocation(name, func){ } } -// Return `fn` with a wrapper that puts its return value through `standardize` -// on the way out. -function makeStandard(standardize, fn){ - return function(){ - return standardize(fn.apply(null, [].slice.call(arguments))); - } -} - function makeArray(valueOrArray) { if (!Array.isArray(valueOrArray)) { return [valueOrArray]; @@ -333,6 +314,60 @@ function makeArray(valueOrArray) { return valueOrArray; } +/** + * recursively adds the middleware in the pages array to array. + */ +function addPageMiddlewareToArray(pages, array) { + if (!pages) return; + + pages.forEach((page) => { + + if (Object.getOwnPropertyNames(page).length === 0) { + throw new Error("Tried to instantiate a page or middleware class that was an empty object. Did you forget to assign a class to module.exports?"); + } + + if (page.middleware) { + addPageMiddlewareToArray(page.middleware(), array); + } + + prepPageForUse(page); + + array.push(page); + }); +} + +function prepPageForUse(page) { + + // Hack around es6 classes. + var proto = Object.getPrototypeOf(new page); + + // We may have already hit this middleware. + if (!proto._reactServerHasAugmented) { + + proto._reactServerHasAugmented = true; + + // Mix in the methods that the page will be able to call on itself. + Object.keys(PAGE_MIXIN).forEach(method => { + + if (proto[method]) { + throw new Error(`PAGE_MIXINS method override: ${page.name}.${method}`); + } + proto[method] = PAGE_MIXIN[method]; + }); + + Object.keys(PAGE_METHODS).forEach(method => { + var orig = proto[method]; + if (orig) { + var standardize = PAGE_METHODS[method][1]; + + proto[method] = function() { + return standardize(orig.apply(this, arguments)); + } + } + }); + } +} + var PageUtil = { PAGE_METHODS, @@ -350,7 +385,19 @@ var PageUtil = { // - PAGE_METHODS // - PAGE_HOOKS // - createPageChain(pages) { + createPageChain(pageConstructor, globalMiddleware) { + + // instantiate the pages we need to fulfill this request. + var classes = pageConstructor.__rsClasses || (pageConstructor.__rsClasses = []); + + if (!classes.length) { + + addPageMiddlewareToArray(globalMiddleware, classes); + addPageMiddlewareToArray([pageConstructor], classes); + } + + var pages = classes.map(Page => new Page); + /* eslint-disable no-loop-func */ // This will be our return value. @@ -360,16 +407,12 @@ var PageUtil = { // var pageChain = Object.create(PAGE_CHAIN_PROTOTYPE); - // Make sure all page classes have been augmented with the - // methods provided by `PAGE_MIXIN`. - pages.forEach(lazyMixinPageUtilMethods); - // Wire up the chained methods. for (var method in PAGE_METHODS){ if (!PAGE_METHODS.hasOwnProperty(method)) continue; - var [defaultImpl, standardize] = PAGE_METHODS[method]; + var [defaultImpl] = PAGE_METHODS[method]; // Take bound methods for each page/middleware that // implements (plus the default implementation), and @@ -385,7 +428,6 @@ var PageUtil = { .filter (page => page[method]) .map (page => page[method].bind(page)) .concat ([defaultImpl]) - .map (makeStandard.bind(null, standardize)) .reduceRight ((next, cur) => cur.bind(null, next)) ); }