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

Only re-render the view(s) you're working on #3959

Merged
merged 6 commits into from Oct 14, 2017

Conversation

Projects
None yet
8 participants
@bep
Member

bep commented Oct 12, 2017

image

Looks promising ... This is 35 times faster than doing the same thing with the current Hugo. There are some missing pieces in this PR (uglyURLs handling etc.), but it holds water.

/cc @spf13 @budparr and gang.

@bep bep added the InProgress label Oct 12, 2017

@axi

This comment has been minimized.

Show comment
Hide comment
@axi

axi Oct 12, 2017

Looks like incremental functionality to me, is it ?

axi commented Oct 12, 2017

Looks like incremental functionality to me, is it ?

@jonathanulco

This comment has been minimized.

Show comment
Hide comment
@jonathanulco

jonathanulco Oct 12, 2017

Is it a kind of incremental ?

jonathanulco commented Oct 12, 2017

Is it a kind of incremental ?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

Looks like incremental functionality to me, is it ?

I've seen the word used before ("when will Hugo implement incremental builds?"), but that term is vaguely defined.

This PR is, however, well defined -- and since this PR is broadcasted on Twitter, I might as well give some context:

Hugo does, in its server mode, already support partial rebuilds. To put it simply: If you change /pages/about.md, only that content page is read and processed, then Hugo does some Site processing (taxonomies etc.) and the full site is rendered.

This PR fixes the part in bold above: We only re-render the pages you work on, i.e. the last n pages you watched in the browser (which obviously also includes the about.md page in the example above).

Doing what I think about as "full incremental builds" is a very hard problem to get right. I have seen other site generators come up with complex solutions to this problem that are filled with corner cases (common issue reports are "incremental builds doesn't work if I ...").

So while this PR isn't perfect, it is extremely simple. If you see some stale content while browsing, I can tell you exactly why.

Member

bep commented Oct 12, 2017

Looks like incremental functionality to me, is it ?

I've seen the word used before ("when will Hugo implement incremental builds?"), but that term is vaguely defined.

This PR is, however, well defined -- and since this PR is broadcasted on Twitter, I might as well give some context:

Hugo does, in its server mode, already support partial rebuilds. To put it simply: If you change /pages/about.md, only that content page is read and processed, then Hugo does some Site processing (taxonomies etc.) and the full site is rendered.

This PR fixes the part in bold above: We only re-render the pages you work on, i.e. the last n pages you watched in the browser (which obviously also includes the about.md page in the example above).

Doing what I think about as "full incremental builds" is a very hard problem to get right. I have seen other site generators come up with complex solutions to this problem that are filled with corner cases (common issue reports are "incremental builds doesn't work if I ...").

So while this PR isn't perfect, it is extremely simple. If you see some stale content while browsing, I can tell you exactly why.

@bep bep changed the title from WIP: Only re-render the view(s) you're working on to Only re-render the view(s) you're working on Oct 12, 2017

@bep bep requested a review from moorereason Oct 12, 2017

@bep bep removed the InProgress label Oct 12, 2017

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

So, @spf13 had some thoughts about this PR "introduces invalid artifacts". I would use the word stale artifact, but that doesn't matter.

So, this PR contains a configurable history; so on change, it re-renders the current and the last n pages you visitied. Combine this with --navigateToChanged and you should get an excellent editing experience.

But if you navigate to a totally different and random page, you will see potentially stale content.

You can then make a random edit and this will be refreshed, but it isn't perfect.

We can fix this if we extend the events that trigger rebuilds to also include pages visited that isn't in the history mentioned above. This should be a rare thing, I suspect, but it would make sure that what the end user sees in development (preview) mode is always up-to-date.

Member

bep commented Oct 12, 2017

So, @spf13 had some thoughts about this PR "introduces invalid artifacts". I would use the word stale artifact, but that doesn't matter.

So, this PR contains a configurable history; so on change, it re-renders the current and the last n pages you visitied. Combine this with --navigateToChanged and you should get an excellent editing experience.

But if you navigate to a totally different and random page, you will see potentially stale content.

You can then make a random edit and this will be refreshed, but it isn't perfect.

We can fix this if we extend the events that trigger rebuilds to also include pages visited that isn't in the history mentioned above. This should be a rare thing, I suspect, but it would make sure that what the end user sees in development (preview) mode is always up-to-date.

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Oct 12, 2017

you will see potentially stale content

But only potentially. I assume the only content that would be stale would be other content impacted by the change (e.g. a page that lists related content, etc).

budparr commented Oct 12, 2017

you will see potentially stale content

But only potentially. I assume the only content that would be stale would be other content impacted by the change (e.g. a page that lists related content, etc).

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

But only potentially. I assume the only content that would be stale would be other content impacted by the change (e.g. a page that lists related content, etc).

That is correct. And when this happens, it should be clear why. The behaviour is well defined. But I think the change I suggest above would be simple enough to add to get rid of this potential annoyance.

Member

bep commented Oct 12, 2017

But only potentially. I assume the only content that would be stale would be other content impacted by the change (e.g. a page that lists related content, etc).

That is correct. And when this happens, it should be clear why. The behaviour is well defined. But I think the change I suggest above would be simple enough to add to get rid of this potential annoyance.

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Oct 12, 2017

Would it be possible to specify certain pages (e.g. home page) to always be built?

budparr commented Oct 12, 2017

Would it be possible to specify certain pages (e.g. home page) to always be built?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

Would it be possible to specify certain pages (e.g. home page) to always be built?

Sure, but I'm not sure what value that would add.

Member

bep commented Oct 12, 2017

Would it be possible to specify certain pages (e.g. home page) to always be built?

Sure, but I'm not sure what value that would add.

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Oct 12, 2017

To lend some predictability to the potential stale problem. Some pages you know will change.

budparr commented Oct 12, 2017

To lend some predictability to the potential stale problem. Some pages you know will change.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

Yea, you have a point. I suggest we add the home page to a list of "always render", then keep the logic as is. Then we can implement my suggestion above later if is this isn't "good enough".

Member

bep commented Oct 12, 2017

Yea, you have a point. I suggest we add the home page to a list of "always render", then keep the logic as is. Then we can implement my suggestion above later if is this isn't "good enough".

@spf13

This comment has been minimized.

Show comment
Hide comment
@spf13

spf13 Oct 12, 2017

Contributor

My vote is that this behavior is available under a "fast-render" flag (pick whatever name you think is appropriate).

I see that their is a flag in the PR but the default is for this behavior to be on.

I'd also suggest that the flag about history count seems unnecessary. Let's just pick a reasonable default... perhaps 20. It will still render very VERY fast and will be more than enough for anyone who enables the "fast render" flag.

Contributor

spf13 commented Oct 12, 2017

My vote is that this behavior is available under a "fast-render" flag (pick whatever name you think is appropriate).

I see that their is a flag in the PR but the default is for this behavior to be on.

I'd also suggest that the flag about history count seems unnecessary. Let's just pick a reasonable default... perhaps 20. It will still render very VERY fast and will be more than enough for anyone who enables the "fast render" flag.

@spf13

This comment has been minimized.

Show comment
Hide comment
@spf13

spf13 Oct 12, 2017

Contributor

To add more color around this. It's a not uncommon use case to run hugo serve on a server against a directory. When files in that directory change it re-renders the site. This new behavior changes things in unexpected ways. It's great for editing, but can break server side and should be enabled explicitly.

Contributor

spf13 commented Oct 12, 2017

To add more color around this. It's a not uncommon use case to run hugo serve on a server against a directory. When files in that directory change it re-renders the site. This new behavior changes things in unexpected ways. It's great for editing, but can break server side and should be enabled explicitly.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

To add more color around this. It's a not uncommon use case to run hugo serve on a server against a directory.

Not in watch mode I'll assume? Hugo server is meant as a development server. Making changes to that behaviour geared towards development should be expected. We will document this in the release notes.

I would prefer fast to be the new default.

Member

bep commented Oct 12, 2017

To add more color around this. It's a not uncommon use case to run hugo serve on a server against a directory.

Not in watch mode I'll assume? Hugo server is meant as a development server. Making changes to that behaviour geared towards development should be expected. We will document this in the release notes.

I would prefer fast to be the new default.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

But I's not important. I'll put in a "vote" in this issue and see what other people say. I agree about the history flag being superflous.

Member

bep commented Oct 12, 2017

But I's not important. I'll put in a "vote" in this issue and see what other people say. I agree about the history flag being superflous.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 12, 2017

Member

Ref. @spf13 's comments above:


Member

bep commented Oct 12, 2017

Ref. @spf13 's comments above:


for j := 0; j < 100; j++ {
wg.Add(1)
go func() {

This comment has been minimized.

@moorereason

moorereason Oct 12, 2017

Contributor
$ megacheck ./common/types/
common/types/evictingqueue_test.go:57:3: the goroutine calls T.Fatal, which must be called in the same goroutine as the test (SA2002)
@moorereason

moorereason Oct 12, 2017

Contributor
$ megacheck ./common/types/
common/types/evictingqueue_test.go:57:3: the goroutine calls T.Fatal, which must be called in the same goroutine as the test (SA2002)

This comment has been minimized.

@bep

bep Oct 12, 2017

Member

Megacheck ... fancy.

@bep

bep Oct 12, 2017

Member

Megacheck ... fancy.

@bep bep added the InProgress label Oct 13, 2017

bep added some commits Oct 13, 2017

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 13, 2017

Member

@moorereason this should be good now, but it would be great with a second set of eyes to look through this.

Member

bep commented Oct 13, 2017

@moorereason this should be good now, but it would be great with a second set of eyes to look through this.

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Oct 13, 2017

I don't know if this is helpful at this stage of your work on this, but I used your branch on a site last night on a big site. There was no difference between the initial build time and subsequent/on-change builds. Though, the overall build times were faster than the current release. If I'm too early on this, just let me know, or I'm otherwise happy to test, etc.

budparr commented Oct 13, 2017

I don't know if this is helpful at this stage of your work on this, but I used your branch on a site last night on a big site. There was no difference between the initial build time and subsequent/on-change builds. Though, the overall build times were faster than the current release. If I'm too early on this, just let me know, or I'm otherwise happy to test, etc.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 13, 2017

Member

If I'm too early on this, just let me know, or I'm otherwise happy to test, etc.

You are not early. At least not as of now (I commited 5 mins ago). The effect of this will be dependent on many things, but if you have 1000 pages -- rendering 10 instead of 1000 should make a big difference; so that means either

  1. You have a mixup in the branches you're using. You should confirm by checking that the new flag is present; hugo server -h | grep disableFastRender
  2. You are testing something I, for some weird reason, have not thought about.

Either way the input is valuable.

Member

bep commented Oct 13, 2017

If I'm too early on this, just let me know, or I'm otherwise happy to test, etc.

You are not early. At least not as of now (I commited 5 mins ago). The effect of this will be dependent on many things, but if you have 1000 pages -- rendering 10 instead of 1000 should make a big difference; so that means either

  1. You have a mixup in the branches you're using. You should confirm by checking that the new flag is present; hugo server -h | grep disableFastRender
  2. You are testing something I, for some weird reason, have not thought about.

Either way the input is valuable.

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Oct 13, 2017

Updated and it's working for me as expected. AND THIS IS AMAZING.

Now that I've used it a bit I think it should not be the default. Most of the developer experience is updating templates and I think—because of the stale content issue—it should be a conscious choice of the developer to turn it on.

Site 1:
Initial build: 2 min. 22 seconds

INITIAL BUILD
5132 regular pages created
22 other pages created
16 paginator pages created
3 series created
4 genre created
total in 141824 ms

SUBSEQUENT BUILDS
Changing content: ~1.2 - ~2 seconds
Changing a template: 1.83 - 3 seconds

Site 2:

INITIAL BUILD
3228 regular pages created
28 other pages created
2 paginator pages created
7 topic created
total in 2592 ms

SUBSEQUENT BUILDS
Changing content: ~0.24 - ~0.27 seconds
Changing a template: ~0.5 - ~0.75 second

budparr commented Oct 13, 2017

Updated and it's working for me as expected. AND THIS IS AMAZING.

Now that I've used it a bit I think it should not be the default. Most of the developer experience is updating templates and I think—because of the stale content issue—it should be a conscious choice of the developer to turn it on.

Site 1:
Initial build: 2 min. 22 seconds

INITIAL BUILD
5132 regular pages created
22 other pages created
16 paginator pages created
3 series created
4 genre created
total in 141824 ms

SUBSEQUENT BUILDS
Changing content: ~1.2 - ~2 seconds
Changing a template: 1.83 - 3 seconds

Site 2:

INITIAL BUILD
3228 regular pages created
28 other pages created
2 paginator pages created
7 topic created
total in 2592 ms

SUBSEQUENT BUILDS
Changing content: ~0.24 - ~0.27 seconds
Changing a template: ~0.5 - ~0.75 second

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 14, 2017

Member

Now that I've used it a bit I think it should not be the default.

I have slept on this a little, and I think you are right. I will sleep on it some more right now.

But I worry that the common man will not "get it". The advanced developer would be able to put two and two together and turn it off when needed.

Member

bep commented Oct 14, 2017

Now that I've used it a bit I think it should not be the default.

I have slept on this a little, and I think you are right. I will sleep on it some more right now.

But I worry that the common man will not "get it". The advanced developer would be able to put two and two together and turn it off when needed.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 14, 2017

Member

I have slept on this a little, and I think you are right. I will sleep on it some more right now.

I have now slept on it and I'm still not sure. Looking at the vote above, I'm not alone.

I may not be the typical Hugo user, but I do use Hugo for a variety of stuff, a mix of template and content editing and I cannot think of one situation where I would not want to have this enabled as a starting point of my editing/development session.

So, if we make this "default OFF", I would create myself a zsh alias and do hugofast server or something. Which would work, but:

  1. I have hugo server pretty fixed in muscle memory and I find it kind of great to have a default setup from something that kind of just works out of the box. So I cd random-hugo-project && hugo server and it just works.
  2. I'm an advanced Hugo/Unix user, so I know about this feature/flag, so I'm in a position to create that alias. Most people don't fit into that category, and many of those will not benefit from this -- because they don't know about it.

If we have it turned on by default, we could (in addition to a bold statement in release notes and docs) add some visible text in the server console: "You are now in Render Fast Mode and may see some stale content on rebuilds. To turn it off, ...".

Member

bep commented Oct 14, 2017

I have slept on this a little, and I think you are right. I will sleep on it some more right now.

I have now slept on it and I'm still not sure. Looking at the vote above, I'm not alone.

I may not be the typical Hugo user, but I do use Hugo for a variety of stuff, a mix of template and content editing and I cannot think of one situation where I would not want to have this enabled as a starting point of my editing/development session.

So, if we make this "default OFF", I would create myself a zsh alias and do hugofast server or something. Which would work, but:

  1. I have hugo server pretty fixed in muscle memory and I find it kind of great to have a default setup from something that kind of just works out of the box. So I cd random-hugo-project && hugo server and it just works.
  2. I'm an advanced Hugo/Unix user, so I know about this feature/flag, so I'm in a position to create that alias. Most people don't fit into that category, and many of those will not benefit from this -- because they don't know about it.

If we have it turned on by default, we could (in addition to a bold statement in release notes and docs) add some visible text in the server console: "You are now in Render Fast Mode and may see some stale content on rebuilds. To turn it off, ...".

@gour

This comment has been minimized.

Show comment
Hide comment
@gour

gour Oct 14, 2017

Contributor

I have now slept on it and I'm still not sure. Looking at the vote above, I'm not alone.

The other static-site-generators are boldly advertising 'icrement-builds', so do not why should Hugo would have 'off' as default?

Contributor

gour commented Oct 14, 2017

I have now slept on it and I'm still not sure. Looking at the vote above, I'm not alone.

The other static-site-generators are boldly advertising 'icrement-builds', so do not why should Hugo would have 'off' as default?

bep added some commits Oct 14, 2017

@bep bep merged commit 60bd332 into gohugoio:master Oct 14, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@anthonyfok

This comment has been minimized.

Show comment
Hide comment
@anthonyfok

anthonyfok Jan 31, 2018

Contributor

Sorry for my very late after-the-fact comment on this PR, but I do have a question:

Does this "Fast Render Mode" imply that changes to the theme isn't rendered? If so, perhaps this side-effect needs to be better documented, as we might have lost a potential new user (@huyingjie) because of it. She wrote:

First, I chose Hugo because of its speed. There is a big drawback that made me gave it up. Hugo will updates automatically when users write posts; it deploys constantly. To achieve this, Hugo saves the contents in the memory [sic]. It is the best choice for people who write posts only. It becomes a big pain for developers since Hugo will delay to update if files of themes are changed. I cannot tell immediately whether my code is working or not and have to wait for Hugo for an unknown length of time.

Then I tried Hexo. My blog was made from Hexo and I like it very much. ...

(from http://yingjiehu.com/a-rsnippet-hexo-theme/ )

Obviously, there is some misunderstanding here (rendering to memory or to disk has nothing to do with the problem @huyingjie ran into, for example), and indeed for a very long time, Hugo has been able to handle changes to theme files and to re-render it correctly and instantaneously.

So, is it true that "Fast Render Mode" currently does not work well with theme editing? Or has it already been fixed recently? (Sorry, I haven't yet done the testing myself.)

Contributor

anthonyfok commented Jan 31, 2018

Sorry for my very late after-the-fact comment on this PR, but I do have a question:

Does this "Fast Render Mode" imply that changes to the theme isn't rendered? If so, perhaps this side-effect needs to be better documented, as we might have lost a potential new user (@huyingjie) because of it. She wrote:

First, I chose Hugo because of its speed. There is a big drawback that made me gave it up. Hugo will updates automatically when users write posts; it deploys constantly. To achieve this, Hugo saves the contents in the memory [sic]. It is the best choice for people who write posts only. It becomes a big pain for developers since Hugo will delay to update if files of themes are changed. I cannot tell immediately whether my code is working or not and have to wait for Hugo for an unknown length of time.

Then I tried Hexo. My blog was made from Hexo and I like it very much. ...

(from http://yingjiehu.com/a-rsnippet-hexo-theme/ )

Obviously, there is some misunderstanding here (rendering to memory or to disk has nothing to do with the problem @huyingjie ran into, for example), and indeed for a very long time, Hugo has been able to handle changes to theme files and to re-render it correctly and instantaneously.

So, is it true that "Fast Render Mode" currently does not work well with theme editing? Or has it already been fixed recently? (Sorry, I haven't yet done the testing myself.)

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jan 31, 2018

Member

@anthonyfok if you have questions on ancient and closed/merged issuges/PRs, you will have better luck taking the questions on the forumm.

Member

bep commented Jan 31, 2018

@anthonyfok if you have questions on ancient and closed/merged issuges/PRs, you will have better luck taking the questions on the forumm.

@anthonyfok

This comment has been minimized.

Show comment
Hide comment
@anthonyfok

anthonyfok Jan 31, 2018

Contributor

@bep Thanks for the reply! I guess I felt conflicted or confused as in I didn't want to make it too public (the forum seems more public to me), and kind of feeling embarrassed because I haven't actually done any testing personally, etc., so probably subconsciously I wasn't really expecting any replies, but needed a place to do a brain-dump... (Maybe I should have just added this to my personal TODO list instead of posting something somewhat random here prematurely.)

Anyhow, thank you for taking the time to reply.

Contributor

anthonyfok commented Jan 31, 2018

@bep Thanks for the reply! I guess I felt conflicted or confused as in I didn't want to make it too public (the forum seems more public to me), and kind of feeling embarrassed because I haven't actually done any testing personally, etc., so probably subconsciously I wasn't really expecting any replies, but needed a place to do a brain-dump... (Maybe I should have just added this to my personal TODO list instead of posting something somewhat random here prematurely.)

Anyhow, thank you for taking the time to reply.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jan 31, 2018

Member

So, is it true that "Fast Render Mode" currently does not work well with theme editing?

No.

Member

bep commented Jan 31, 2018

So, is it true that "Fast Render Mode" currently does not work well with theme editing?

No.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment