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

feat(image): postAsset to prepend post's relative path #159

Merged
merged 12 commits into from
Aug 21, 2020

Conversation

curbengh
Copy link
Contributor

@curbengh curbengh commented Aug 13, 2020

Fixes hexojs/hexo#4454 cc @v-tawe

How to use:

#_config.yml
post_asset_folder: true
marked:
  prependRoot: true
  postAsset: true

Based on asset_img tag plugin.

@coveralls
Copy link

coveralls commented Aug 13, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 189360a on curbengh:post-asset into 0000e3a on hexojs:master.

if (data.path && this.config.post_asset_folder && this.config.marked.prependRoot && this.config.marked.postAsset) {
const Post = this.model('Post');
const source = relative(this.source_dir, data.path);
const post = Post.findOne({ source });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

may support Page in future.

lib/renderer.js Outdated Show resolved Hide resolved
lib/renderer.js Outdated Show resolved Hide resolved
@curbengh curbengh marked this pull request as draft August 14, 2020 04:27
@curbengh curbengh marked this pull request as ready for review August 15, 2020 08:54
@curbengh
Copy link
Contributor Author

curbengh commented Aug 15, 2020

Ready for review, tested ![](img.png) and ![](assets/img.png) in Ubuntu and Windows, ![](assets\img.png) will also works in both platforms.

lib/renderer.js Outdated Show resolved Hide resolved
@curbengh curbengh requested a review from SukkaW August 17, 2020 08:53
@curbengh curbengh changed the title feat: postAsset to prepend post's relative path feat(image): postAsset to prepend post's relative path Aug 17, 2020
@curbengh
Copy link
Contributor Author

May support prepend to path in future, similar function to asset_path tag plugin.

lib/renderer.js Outdated Show resolved Hide resolved
@SukkaW
Copy link
Member

SukkaW commented Aug 17, 2020

Is the performance being impacted? hexo.model('PostAsset').findOne could be slow.

@curbengh
Copy link
Contributor Author

Is the performance being impacted?

I suspect it will, hence this feature is disabled by default. i'm benchmarking 500 posts + 100 asset images for each post.

@curbengh
Copy link
Contributor Author

curbengh commented Aug 18, 2020

I scaled down my test to 100 posts with 100 asset images in each post, my machine couldn't handle 500 posts.
Each post has links to those images, plus 100 site image (e.g. ![](/foo/<1-100>.png)). When this feature is enabled, hexo generate became 6x slower.

Significant slowdown was due to PostAsset.findOne()'s query on the site image. So I made a small change to skip irrelevant paths (e.g. http://foo.com, #heading-id, /foo/1.png, \foo\1.png).

It's now 3x slower. I also added a note about the processing time.

lib/renderer.js Outdated
if (!/^(\/|\\)/.test(href) && postId) {
const PostAsset = hexo.model('PostAsset');
// slug requires platform-specific path
const asset = PostAsset.findOne({ post: postId, slug: href.replace(/\/|\\/g, sep) });
Copy link
Member

@SukkaW SukkaW Aug 18, 2020

Choose a reason for hiding this comment

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

Instead of using findOne, is it possible to use findById directly?

https://github.com/hexojs/hexo/blob/master/lib/plugins/processor/post.js

Also, each post should only find PostAsset for once (not for every image). A cache should be implemented.

Copy link
Member

@SukkaW SukkaW Aug 18, 2020

Choose a reason for hiding this comment

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

const { Cache } = require('hexo-util');
const postAssetCache = new Cache();

// ...

const asset = postAssetCache.apply(postId, () => PostAsset.findOne({ post: postId, slug: href.replace(/\/|\\/g, sep) }));

And see if there is any performance improvement.

It could also be:

const { Cache } = require('hexo-util');
let postAssetCache;

// ...

// Only init cache when prependRoot is enabled
if (!relative_link && prependRoot) {
  postAssetCache = postAssetCache || new Cache();
}

const asset = postAssetCache.apply(postId, () => PostAsset.findOne({ post: postId, slug: href.replace(/\/|\\/g, sep) }));

lib/renderer.js Outdated Show resolved Hide resolved
@curbengh
Copy link
Contributor Author

curbengh commented Aug 18, 2020

Instead of using findOne, is it possible to use findById directly?

I tried and it does work.

Also, each post should only find PostAsset for once

in my test, all 10,000 asset images have unique path, so PostAsset needs to run for every relevant path to make sure it is a valid asset path (the path links to an existing asset image) before prepending the post path.

Alternatively, it is possible to get rid of PostAsset and simply process all relevant path regardless of validity. Aside from not checking for validity, user must be consistent in specifying path, e.g. site image must starts with a slash (/site/assets/logo.png) while asset image must not starts with a slash (screenshot.png or assets/screenshot.png)

(relevant path is any path that does not start with a forward/backward slash)

This also means, with PostAsset, when a user embeds a site image without starting with a slash (site/assets/logo.png), since PostAsset knows that it is not a post asset, it will simply return falsy, and then url_for() will prepend a slash or /root/ to it.

We could offer two modes:

# _config.yml
marked:
  postAsset: 'accurate|fast'

@curbengh
Copy link
Contributor Author

curbengh commented Aug 18, 2020

PostAsset.findById is fast, now this feature only slows down by 2s (12s to 14s), but probably is caused by Post.findOne. I'll test by removing PostAsset.


Edit: removing PostAsset didn't yield significant improvement and the difference was hardly noticeable.

@SukkaW
Copy link
Member

SukkaW commented Aug 18, 2020

@curbengh There are still some issue:

  • In order to finish the feature, we introduces too many dirty code (even hexo database) into the renderer
  • Only hexo-renderer-marked would benefit while other renderer (even post not written in markdown) still not supported

I would rather bring up the feature based on after_post_render filter.

@curbengh
Copy link
Contributor Author

curbengh commented Aug 18, 2020

  1. new Renderer(this); looks more readable to me, compared to previous approach Object.assign(siteCfg); in previous approach, renderer options, hexo.config.marked, hexo.config.url are all bundled up in options and it can be confusing to differentiate them.
  2. hexo-renderer-marked are installed by default and most users use it.
  3. filter is slower due to finding <img>.
  • this is also why the following feature is introduced last time:
marked:
  external_link:
    enable: false
    exclude: []
    nofollow: false

@SukkaW
Copy link
Member

SukkaW commented Aug 18, 2020

  1. hexo-renderer-marked are installed by default and most users use it.
  2. filter is slower due to finding <img>.

It should be no slower than querying warehouse. Also, every renderer that Hexo is supported will be benefited.

@curbengh
Copy link
Contributor Author

curbengh commented Aug 21, 2020

It should be no slower than querying warehouse.

filter still requires querying warehouse to skip non-post-asset, String.replace(/<img>/g) + PostAsset.findById().
whereas this PR is Post.findOne() + PostAsset.findById().

The benchmark showed there's hardly any different when enabling this feature, the slowdown actually comes from post_asset_folder (due to copying 10,000 files), not warehouse.

@curbengh curbengh merged commit 8e1104c into hexojs:master Aug 21, 2020
@curbengh curbengh deleted the post-asset branch August 21, 2020 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image cannot be load by relative path using markdown syntax
3 participants