Call setSitecoreContext silently during SSR

[erzr]

During some profiling Server-Side Rendering of the sample application setSitecoreContext stood out quite a bit and it seems like that when setSitecoreContext is called with the initial SSR state, this triggers subscribers to be called, which ends up calling forceUpdate.

This doesn't seem to be necessary during SSR and skipping the notification of subscribers seems to increase throughput of SSR by ~30% in my testing:

Before:

  Scenarios launched:  100
  Scenarios completed: 100
  Requests completed:  2000
  Mean response/sec: 147.49
  Response time (msec):
    min: 20.9
    max: 786.9
    median: 590.9
    p95: 716.4
    p99: 766.2
  Scenario counts:
    0: 100 (100%)
  Codes:
    200: 2000

After

  Scenarios launched:  100
  Scenarios completed: 100
  Requests completed:  2000
  Mean response/sec: 210.08
  Response time (msec):
    min: 61.3
    max: 572.2
    median: 411.2
    p95: 462.1
    p99: 493.5
  Scenario counts:
    0: 100 (100%)
  Codes:
    200: 2000

This could use some test coverage, which I can add if this change is adopted. Open to any/all questions here.

Motivation

Increase performance of Server-Side Rendering.

How Has This Been Tested?

Limited local testing, using https://artillery.io/ to simulate load against proxy.
artillery quick --count 100 -n 20 http://localhost:3000

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update (non-breaking change; modified files are limited to the /docs directory)

Checklist:

• I have read the Contributing guide.
• My code follows the code style of this project.
• My code/comments/docs fully adhere to the Code of Conduct.
• My change is a code change and it requires an update to the documentation.
• My change is a documentation change and it requires an update to the navigation.

[sc-illiakovalenko]

@erzr I think, in this case subscribers will not be called in server side at all, am I right? Seems that it's correct.
Also, please, change base branch to: release/15.0.0

[sc-illiakovalenko]

@aweber1 @nickwesselman @kamsar @erzr Guys, please, look at my comment, maybe you can advice something.
I dived into this topic, and I'm thinking about question:
Do we need an Array of subscribers in SitecoreContextFactory? Now I see that on client side, we have always 1 subscriber - current opened application in browser.
But on the server side, subscribers pushed every time to the Array, but we don't need it, we need only one subscriber on every request

I propose to re implement SitecoreContextFactory like this, I tested it a little bit, seems that it's working as usually.
So before sending to client - we will have correct subscriber, it will be pushed on client. Probably this change can improve performance.
Correct me If I wrong, maybe i miss something

export class SitecoreContextFactory {
  subscriber: any;
  // subscribers: any[];
  context: any;

  constructor() {
    this.context = {
      pageEditing: false,
    };
  }

  getSitecoreContext = () => {
    return this.context;
  }

  subscribeToContext = (func: any) => {
    this.subscriber = (value: any) => func(value);
  }

  unsubscribeFromContext = () => {
    this.subscriber = undefined;
    // const index = this.subscribers.indexOf(func);
    // if (index >= 0) {
    //   this.subscribers.splice(index, 1);
    // }
  }

  setSitecoreContext = (value: any, silent: boolean = false) => {
    this.context = value;
    
    if (!silent) {
      this.subscriber(value)
      // this.subscribers.forEach((func) => func(value));
    }
  }
}

[nickwesselman]

Theoretically you could use the SitecoreContext in a more targeted manner right? In which case you'd need more than one subscriber? Maybe that's the issue here -- rather than wrapping the whole app, and I assume causing a re-render of everything when context updates, should we instead be using it within individual components when the context data is needed?

And are we saying we somehow end up w/ more than one subscriber in SSR though with usage in the sample app, and thus multiple calls to forceUpdate?

Regarding the need for this in SSR, a use case to test would potentially be use checking the page mode to display something only when in the Experience Editor. Is that page mode accurate if we disable the update to the subscribers?

[sc-illiakovalenko]

I tested it and understood, that we can't do such changes.
Because:

1. If we skip setup of context on client side, we don't have a context inside the components
2. We have to setup context and re render components that are wrapped by withSitecoreContext

Try to open page directly: /styleguide, you will have issue
But when you navigate from Home to Styleguide, it's working fine, because we don't have silent in setSitecoreContext in updateRouteData function.

[nickwesselman]

Maybe related, our internal profiling has revealed a potential memory leak in SitecoreContext... is the actual issue here that we are not releasing subscribers?

[nickwesselman]

Looks like we are leaking subscribers because componentWillUnmount is not called during SSR. Not sure what the proper way to do this is in SSR.

https://github.com/Sitecore/jss/blob/dev/packages/sitecore-jss-react/src/components/SitecoreContext.tsx#L76

See facebook/react#3714

[sc-illiakovalenko]

@nickwesselman You are right. I was thinking about it. So it's a cause, why i'm trying to get rid of subscribers and try to use one subscriber. I'm preparing PR, I'll notify you, when I'll open it. I tried to build flame graph when I use one subscriber and it's totally different, SitecoreContext doesn't hot. And yes, I logged subscribers on server side, it cannot be unsubscribed.

You can look at: https://itnext.io/tips-for-server-side-rendering-with-react-e42b1b7acd57
As you can see, on the server side, these cycles is working:

• constructor
• getDerivedStateFromProps
• componentWillMount (deprecated)

[sc-illiakovalenko]

Opened PR related to my idea: #414

[sc-illiakovalenko]

I close this PR, as have already merged #414 that includes refactoring of SitecoreContext, that fixes memory leak issue