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

Implement lazy page building for markbind serve #1038

Merged
merged 1 commit into from
Mar 8, 2020

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented Feb 11, 2020

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Enhancement to an existing feature

Resolves #1007

What is the rationale for this request?

To implement the following two things

Lazily building the initial serve

  • the loading spinner is shown when the user navigates to pages that have not been built at least once, otherwise the old page is shown while the new page is being built
  • only the landing page is built, taking about ~2.5s

Lazily rebuilding affected source files

Editing text.md of software engineering topic of 2103 website:

  • normal live reloading : 9 pages rebuilt, taking an average of about 7.5s
  • lazy live reloading : 1 page rebuilt, taking around 0.8s

Gifs are here to keep things lean: #1038 (comment)

The main caveat is that heading indexing for unbuilt / unrebuilt pages will not be run, causing fewer than expected search results to show up, until the user navigates to that page.
Hence, the possible search results are limited to the starting page at first, while slowly extending to all pages as the user navigates to them.

In the long run, the savings in rebuild times from rebuilding the page the user is working on only should greatly outweight the "extra time before serving the page".

What changes did you make? (Give an overview)

  • Repurposed the -o option for lazy live reloading, which's behaviour is:
    • only builds the landing page on startup, lazily building other pages as the user navigates to them.
    • lazily builds only the current page if needed, when a file change is detected, and "flags" affected pages for lazily building letter
  • Introduced a middleware to detect user requests for a page, allowing processing for that page in the site instance:
    • setting the currentPageViewed
    • rebuilding the page being navigated to before serving it if : it was never built before, or some file changed before that is a dependency of it.
  • Update and organize the cli commands documentation page with some panels
  • Introduce a raw html loading spinner page to serve when the page being navigated to needs to be built / rebuilt
  • Simple performance logging for live reload timings, including a recommendation message to use lazy loading if the timings crosses certain thresholds
    • currently, this message repeats itself whenever the threshold is crossed. A seperate PR / issue can look into a package-wide user config of sorts, where this recommendation will be disabled after showing it one time
    • this PR is getting slightly large otherwise, I could implement it here if preferred though

Provide some example code that this change will affect:

const lazyReloadMiddleware = function (req, res, next) {
  const isHtmlFile = req.url.endsWith('.html');

  if (isHtmlFile) {
    const isDynamicIncludeHtmlFile = req.url.endsWith('._include_.html');

     if (!isDynamicIncludeHtmlFile) {
       const urlWithoutBaseUrl = req.url.replace(config.baseUrl, '');
       const urlWithoutExtension = fsUtil.removeExtension(urlWithoutBaseUrl);

       // changeCurrentPage initiates the page rebuild if needed
       const didInitiateRebuild = site.changeCurrentPage(urlWithoutExtension);
       if (didInitiateRebuild) {
         // the request is redirected to the loading spinner page in the meantime
         req.url = ensurePosix(path.join(config.baseUrl || '/', LAZY_LOADING_SITE_FILE_NAME));
       }
    }
  }

  next();
};

Is there anything you'd like reviewers to focus on?
na

Testing instructions:
markbind serve -o <file> should build only the <file> specified ( by default landing page index.md if none ) and start the live server.

  • Changing any source file that a page is dependent on, and being viewed, should rebuild the page as per normal.
  • Changing any source file that a page is dependent on, but not being viewed, should not rebuild the page, but "flag" it to do so later. Also, all page source files except <file> should be "flagged" when initially running markbind serve -o.
    • Navigating to it later however, should serve the loading page while the page is being rebuilt. The browser should then automatically refresh to show the new page once rebuilt.
  • Navigating to pages that are not "flagged" should not show the loading page and should immediately serve the page.

Proposed commit message: (wrap lines at 72 characters)
Implement lazy page building for markbind serve

During live reload, changes to a source file rebuilds all files that are
dependent on it.
In addition, rebuilding all dependent source files slows the live reload
process, leading to a less pleasant user experience.

In addition, when running markbind serve, all pages are built and
rendered initially.
This significantly slows down the time to first page load, which can
be substantial when the number of source files is huge.

Let's implement lazy page building in these operations, which is opt-in
using the existing -o or --one-page flags for the markbind
serve command.

Changes to a source file will only rebuild pages that are dependent on
it, and is currently being viewed by the author. Other pages not being
viewed by the author are rebuilt when the author navigates to them.
In addition, only the landing page is built initially when serving.
Subsequent pages are built when the author navigates to them.

@damithc
Copy link
Contributor

damithc commented Feb 11, 2020

This can really help improve QoL of authors, if the eventually makes it to the production version. 👍

@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Feb 11, 2020

This can really help improve QoL of authors, if the eventually makes it to the production version. 👍

Would the caveat of search results being incorrect be an acceptable compromise?

The problem would be exacerbated if the extension is implemented as well ( in this case the user author is left with much fewer search results than there should have been )

( that said, the author could still use the sitenav and pagenav when writing pages to navigate around quickly )

@acjh
Copy link
Contributor

acjh commented Feb 11, 2020

  1. How about if this were a flag, e.g. --lazy?
  2. I wonder if build could be passed a flag that specifies storing metadata and then serve could be passed a corresponding flag that skips the initial build.
    • This would additionally allow a user to serve a site that has custom post-build changes (maybe for some workaround) that is currently possible with build(+change)+deploy.

@ang-zeyu
Copy link
Contributor Author

  1. How about if this were a flag, e.g. --lazy?

Yup, the current flag I used is -l or --lazy-reload, which is in line with -f & --force-reload.

We could document this as a feature for "extremely large sites", where the benefits of build times would outweigh the compromise in search results.

  1. I wonder if build could be passed a flag that specifies storing metadata and then serve could be passed a corresponding flag that skips the initial build.

    • This would additionally allow a user to serve a site that has custom post-build changes (maybe for some workaround) that is currently possible with build(+change)+deploy.

Hmm, I might have misunderstood what you meant, but dosen't serve currently generate the site "on its own" ( site.generate() ), rather than calling markbind build?

Along those lines I think its definitely feasible to pass a flag to serve for that though

@ang-zeyu ang-zeyu force-pushed the lazy-live-reloading branch 3 times, most recently from 77b338c to 03aa0db Compare February 12, 2020 04:06
@ang-zeyu
Copy link
Contributor Author

Updated with the extension, and cleaned up some things.

Lazily building the initial serve

  • the loading spinner is shown when the user navigates to pages that have not been built at least once, otherwise the old page is shown while the new page is being built
  • only the landing page is built, taking about ~2.5s
    lazyLiveReloadDemo1
    ( the ghosted spinner is a bug with my gif software )

Demo of lazy live reload:

lazyLiveReloadDemo2

@ang-zeyu ang-zeyu changed the title [RFC] Implement lazy live reloading Implement lazy page building for markbind serve Feb 12, 2020
@damithc
Copy link
Contributor

damithc commented Feb 12, 2020

I think missing search results is an acceptable compromise as long as the user is told about it and as long as there is a way to opt out of lazy live loading.
We already have a flag -o to limit live loading to one specific file. Perhaps we can combine the two, at least from user POV? In fact, we might not even need that feature after adding lazy live loading?

@ang-zeyu
Copy link
Contributor Author

We already have a flag -o to limit live loading to one specific file. Perhaps we can combine the two, at least from user POV? In fact, we might not even need that feature after adding lazy live loading?

Combining it is definitely feasible, and removing it should be problem free as well since the implementation is rather "independent".

Personally, I prefer removing it, though it may be helpful to "start the lazy mode" on a particular page to hasten things a little.
Which should we go with?

@ang-zeyu ang-zeyu changed the title Implement lazy page building for markbind serve [WIP] Implement lazy page building for markbind serve Feb 12, 2020
@damithc
Copy link
Contributor

damithc commented Feb 12, 2020

Personally, I prefer removing it, though it may be helpful to "start the lazy mode" on a particular page to hasten things a little.
Which should we go with?

Is it possible to make it like this?
-o: starts in lazy mode
-o filename: limits loading to one file only

@ang-zeyu
Copy link
Contributor Author

Is it possible to make it like this?
-o: starts in lazy mode
-o filename: limits loading to one file only

Should be possible, though just to clarify, -o is equivalent to -o index.md right?
( If the user does something like -o contents/topic1.md, they should be able to also navigate to and lazily build other pages from there as well )

I'll go with the existing -o over introducing a -l option then!

@damithc
Copy link
Contributor

damithc commented Feb 12, 2020

Should be possible, though just to clarify, -o is equivalent to -o index.md right?

Yes.

The other question to ask is whether lazy live loading should be the default (i.e., we provide a flag to opt out of it).

@ang-zeyu
Copy link
Contributor Author

Yes.

The other question to ask is whether lazy live loading should be the default (i.e., we provide a flag to opt out of it).

I think it should be optionally activated through -o.
The default behaviour should be one with everything working correctly ( search results )

  • What we could do is monitor the logged build times, if it is excessively large, we can print a one time ( per project / system ) recommendation message to the user, to recommend the -o option.
  • A good threshold may be 30s for the build times, or 5s for reload times

A small thing:
When we use serve -o command, upon navigating to a page that has not been built before, the loading spinner page seen above will be shown.
For the case that the page has been built before, but is "outdated", should we

  • show the old page ( current behaviour, this would give the impression that the page "refreshes twice" )
  • or the spinner page as well?
  • or inject a message into the old page to inform the user that it is outdated and currently rebuilding, but this would require some additional amount of file i/o and a little processing, which I'm not sure is worth it. ( the page might have already been almost rebuilt by the time this is done )

@damithc
Copy link
Contributor

damithc commented Feb 12, 2020

I think it should be optionally activated through -o.
The default behaviour should be one with everything working correctly ( search results )

  • What we could do is monitor the logged build times, if it is excessively large, we can print a one time ( per project / system ) recommendation message to the user, to recommend the -o option.
  • A good threshold may be 30s for the build times, or 5s for reload times

Sure, that works.

When we use serve -o command, upon navigating to a page that has not been built before, the loading spinner page seen above will be shown.
For the case that the page has been built before, but is "outdated", should we

  • show the old page ( current behaviour, this would give the impression that the page "refreshes twice" )
  • or the spinner page as well?
  • or inject a message into the old page to inform the user that it is outdated and currently rebuilding, but this would require some additional amount of file i/o and a little processing, which I'm not sure is worth it. ( the page might have already been almost rebuilt by the time this is done )

I think we should show spinner, but open to other suggestions.

@ang-zeyu ang-zeyu force-pushed the lazy-live-reloading branch 2 times, most recently from f65bbd1 to 274a57d Compare February 12, 2020 09:38
@ang-zeyu ang-zeyu changed the title [WIP] Implement lazy page building for markbind serve Implement lazy page building for markbind serve Feb 12, 2020
@ang-zeyu ang-zeyu force-pushed the lazy-live-reloading branch 3 times, most recently from bde1dd3 to 610be36 Compare February 12, 2020 14:03
@ang-zeyu
Copy link
Contributor Author

Ready for review

I think we should show spinner, but open to other suggestions.

I've changed it to show the spinner, which I think is better as well.
I'm open to suggestions as well though, but we can always change it the other way around if preferred easily, in the future

@ang-zeyu ang-zeyu force-pushed the lazy-live-reloading branch 5 times, most recently from 769e8ad to 1e3de91 Compare February 17, 2020 10:38
@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Feb 17, 2020

Updated on latest master with small fix for file addition / deletion


Will comment again if there are changes / conflicts while keeping this updated

@ang-zeyu ang-zeyu force-pushed the lazy-live-reloading branch 4 times, most recently from a75de0e to 49e33e2 Compare February 26, 2020 10:24
@ang-zeyu ang-zeyu force-pushed the lazy-live-reloading branch 2 times, most recently from 1e9f302 to 7357f9b Compare March 4, 2020 13:19
index.js Outdated
next();
};

serverConfig.middleware = [lazyReloadMiddleware];
Copy link
Member

Choose a reason for hiding this comment

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

Initialize an empty array for middleware during the initialization of serverConfig, then replace this with a push instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated; Thanks for looking through this!

During live reload, changes to a source file rebuilds all files that are
dependent on it.

In addition, rebuilding all dependent source files slows the live reload
process, leading to a less pleasant user experience.

In addition, when running markbind serve, all pages are built and
rendered initially.
This significantly slows down the time to first page load, which can
be substantial when the number of source files is huge.

Let's implement lazy page building in these operations, which is opt-in
using the existing -o <file> or --one-page <file> flags for the markbind
serve command.

Changes to a source file will only rebuild pages that are dependent on
it, and is currently being viewed by the author. Other pages not being
viewed by the author are rebuilt when the author navigates to them.
In addition, only the landing page is built initially when serving.
Subsequent pages are built when the author navigates to them.
@yamgent yamgent added this to the v2.11.1 milestone Mar 7, 2020
@yamgent yamgent merged commit 1451bd5 into MarkBind:master Mar 8, 2020
@yamgent yamgent added the pr.Enhancement 📈 Enhancement to an existing feature label Mar 8, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 9, 2020
* 'master' of https://github.com/MarkBind/markbind:
  2.12.0
  Update outdated test files
  Update vue-strap version to v2.0.1-markbind.37
  Fix refactor to processDynamicResources (MarkBind#1092)
  Implement lazy page building for markbind serve (MarkBind#1038)
  Add warnings for conflicting/deprecated component attribs (MarkBind#1057)
  Allow changing parameter properties (MarkBind#1075)
  Custom timezone for built-in timestamp (MarkBind#1073)
  Fix reload inconsistency when updating frontmatter (MarkBind#1068)
  Implement an api to ignore content in certain tags (MarkBind#1047)
  Enable AppVeyor CI (MarkBind#1040)
  Add heading and line highlighting to code blocks (MarkBind#1034)
  Add dividers and fix bug in siteNav (MarkBind#1063)
  Fixed navbar no longer covers modals (MarkBind#1070)
  Add copy code-block plugin (MarkBind#1043)
  Render plugins on dynamic resources (MarkBind#1051)
  Documentation for Implement no-* attributes for <box>  (MarkBind#1042)
  Migrate to bootstrap-vue popovers (MarkBind#1033)
  Refactor preprocess and url processing functions (MarkBind#1026)
  Add pageNav to Using Plugins Page (MarkBind#1062)
@damithc
Copy link
Contributor

damithc commented Mar 9, 2020

This feature is great 👍 I'm going to use the -o option all the time now. Thanks @ang-zeyu and reviewers.

marvinchin pushed a commit that referenced this pull request Apr 10, 2020
During live reload, changes to a source file rebuilds all files that are
dependent on it.

In addition, rebuilding all dependent source files slows the live reload
process, leading to a less pleasant user experience.

In addition, when running markbind serve, all pages are built and
rendered initially.
This significantly slows down the time to first page load, which can
be substantial when the number of source files is huge.

Let's implement lazy page building in these operations, which is opt-in
using the existing -o <file> or --one-page <file> flags for the markbind
serve command.

Changes to a source file will only rebuild pages that are dependent on
it, and is currently being viewed by the author. Other pages not being
viewed by the author are rebuilt when the author navigates to them.
In addition, only the landing page is built initially when serving.
Subsequent pages are built when the author navigates to them.
@ang-zeyu ang-zeyu mentioned this pull request Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr.Enhancement 📈 Enhancement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lazy live reloading
4 participants