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

Possible issue with processing pages (promises) #6

Closed
markomanninen opened this Issue Jan 10, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@markomanninen
Contributor

markomanninen commented Jan 10, 2016

I found this problem on my other plugin using a similar pre-processing structure that is used on image-captions. While Q.all prevents the page hook to be called before the init hook is done, processPage calls in the init hook are still async. This means that even navigation items (files) are iterated in order, page processing will be async. This may cause image lists to become out of order. Using additional promise code (reduce thing), this possible problem can be solved.

markomanninen@03c98dc

The problem won't be visible on short test pages with few images, I guess. But on a real book project, with hundred pages and tons of pictures, it will become apparent.

You may want to recheck this scenario, Tomas.

@todvora

This comment has been minimized.

Show comment
Hide comment
@todvora

todvora Jan 13, 2016

Owner

Hi Marko,
Thank you for the warning and your solution. I'll try to write some tests for this scenario and integrate your fix. First thought - if we serialize all the calls, will be the processing noticeably slower? Any experience?

Best regards,
Tomas

Owner

todvora commented Jan 13, 2016

Hi Marko,
Thank you for the warning and your solution. I'll try to write some tests for this scenario and integrate your fix. First thought - if we serialize all the calls, will be the processing noticeably slower? Any experience?

Best regards,
Tomas

@markomanninen

This comment has been minimized.

Show comment
Hide comment
@markomanninen

markomanninen Jan 14, 2016

Contributor

I have not seen any speed decrease, by eye. Thought I have not profiled programmically either. Is there any easy to use build profiler / timer for GitBook project? On the other hand the output is static anyway, so speed is not that critical in my opinion.

Contributor

markomanninen commented Jan 14, 2016

I have not seen any speed decrease, by eye. Thought I have not profiled programmically either. Is there any easy to use build profiler / timer for GitBook project? On the other hand the output is static anyway, so speed is not that critical in my opinion.

@todvora

This comment has been minimized.

Show comment
Hide comment
@todvora

todvora Jan 14, 2016

Owner

I merged your change and all the tests are passing. On my current test cases isn't any slowdown observable. Thank you for this contribution Marko!

Owner

todvora commented Jan 14, 2016

I merged your change and all the tests are passing. On my current test cases isn't any slowdown observable. Thank you for this contribution Marko!

@todvora

This comment has been minimized.

Show comment
Hide comment
@todvora

todvora Jan 14, 2016

Owner

Regarding build profiler - you can try the gitbook-tester in the same manner as in this plugin. Every test case outputs total time of execution. So it should be possible to see a potential slowdown, if you generate big enough book and let it build by the tester.

Owner

todvora commented Jan 14, 2016

Regarding build profiler - you can try the gitbook-tester in the same manner as in this plugin. Every test case outputs total time of execution. So it should be possible to see a potential slowdown, if you generate big enough book and let it build by the tester.

@todvora

This comment has been minimized.

Show comment
Hide comment
@todvora

todvora Feb 3, 2016

Owner

Hi Marko,
The latest version of your code worked ok for me and passed all tests. Unfortunately there has been a error introduced (#7).

I've created new test case, that validates exactly your observation with mixed up order of image global indexes. The problem could be observed immediately.

Then I changed the logic around Q.all little bit. First it parses all the images in parallel. Then they are sorted according to the navigation object. During the refactoring, some other things has been slightly changed. Mainly scoping (this/that), global images variable and so one.

It would be cool, if you had some time and could check those changes and source code. There are lots of features, you designed and you know best, if they work as expected.

Thank you very much!

Best regards,
Tomas

Owner

todvora commented Feb 3, 2016

Hi Marko,
The latest version of your code worked ok for me and passed all tests. Unfortunately there has been a error introduced (#7).

I've created new test case, that validates exactly your observation with mixed up order of image global indexes. The problem could be observed immediately.

Then I changed the logic around Q.all little bit. First it parses all the images in parallel. Then they are sorted according to the navigation object. During the refactoring, some other things has been slightly changed. Mainly scoping (this/that), global images variable and so one.

It would be cool, if you had some time and could check those changes and source code. There are lots of features, you designed and you know best, if they work as expected.

Thank you very much!

Best regards,
Tomas

@markomanninen

This comment has been minimized.

Show comment
Hide comment
@markomanninen

markomanninen Feb 17, 2016

Contributor

Hi Tomas,

brilliant work you have done! I took a look (actually few times to the code). Refactoring looks nice, it clearer to read now. Actual fix to the map/reduce dilemma is still strange. Did you find the precise point when reduce index error was found? What caused it? I'm glad you could verify the number order issue on big lists. I had too limited time to test with given open source books, but it looks reasonable that image list is sorted at the end according to menu order.

Have a great time with projects!
-Marko

Contributor

markomanninen commented Feb 17, 2016

Hi Tomas,

brilliant work you have done! I took a look (actually few times to the code). Refactoring looks nice, it clearer to read now. Actual fix to the map/reduce dilemma is still strange. Did you find the precise point when reduce index error was found? What caused it? I'm glad you could verify the number order issue on big lists. I had too limited time to test with given open source books, but it looks reasonable that image list is sorted at the end according to menu order.

Have a great time with projects!
-Marko

@todvora

This comment has been minimized.

Show comment
Hide comment
@todvora

todvora Mar 1, 2016

Owner

Hi Marko,
Thank you! I haven't been able to find the exact reason, why the map/reduce failed. Nevertheless the bug could be verified on books from #7. I wanted to limit number of side effects (global vars) from our processing pipeline too, so I rather invested time into refactoring.

With Gitbook v3.0 are the internals and API totally different, so it will be needed some kind of abstraction and refactoring again, to maintain compatibility with 2. and 3. releases. Stay tuned :-)

Best regards,
Tomas

Owner

todvora commented Mar 1, 2016

Hi Marko,
Thank you! I haven't been able to find the exact reason, why the map/reduce failed. Nevertheless the bug could be verified on books from #7. I wanted to limit number of side effects (global vars) from our processing pipeline too, so I rather invested time into refactoring.

With Gitbook v3.0 are the internals and API totally different, so it will be needed some kind of abstraction and refactoring again, to maintain compatibility with 2. and 3. releases. Stay tuned :-)

Best regards,
Tomas

@todvora

This comment has been minimized.

Show comment
Hide comment
@todvora

todvora May 26, 2016

Owner

Refactoring done, lots of changes included. Closing this issue now, but feel free to reopen it or create a new one, if there is something more to solve.

Owner

todvora commented May 26, 2016

Refactoring done, lots of changes included. Closing this issue now, but feel free to reopen it or create a new one, if there is something more to solve.

@todvora todvora closed this May 26, 2016

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