webtoon / psd

Fast zero-dependency PSD parser for the web and Node.js
https://webtoon.github.io/psd
MIT License
1.21k stars 55 forks source link

feature: implement EngineData parsing #43

Closed scoiatael closed 2 years ago

scoiatael commented 2 years ago

fixes https://github.com/webtoon/psd/issues/6

This feature should be more or less finished - during the code review I'll run it on sample designs to verify parsing works consistently.

scoiatael commented 2 years ago

No hard feelings, but next time you could mention you are willing to give a feature one more try - I could have implemented something else in the same time and library would've benefited from that :)


As for this PR - I implemented fix to the CJK parsing bug and added test for it. I also migrated parser to stack-based - to make optimizations a little easier (now flamegraphs can properly merge similar invocations because stack looks largely the same).

That said, I couldn't find any meaningful optimizations (more that ~5% on Node.js). I suspect your hand-written lexer simply behaves better that what is generated from the async-function in my case :)


As for next steps - I'm literally indifferent as to which implementation ends up on master - as long as any does :) As you are the maintainer, the choice is yours (and your colleagues). The code in this PR is also yours to use as you see fit - e.g. for integrating your library if that speeds up development and such is the choice. Just let me know ;)


One final question: would it make sense to re-write this part in Rust? Since it looks like parsing EngineData might take a large portion of total design parsing time.

pastelmind commented 2 years ago

Thank you. I handled the situation poorly, and will not make the same mistake again.

I doubt it would be appropriate to convert the EngineData parser to Rust/WebAssembly. Decoding layer images is by far the most time- and memory-intensive task (hundreds of milliseconds per layer). Parsing an EngineData document with JS takes < 1ms when optimized, and perf gains from Rust+WebAssembly would be barely noticeable.

scoiatael commented 2 years ago

Ran some benchmarks; looks like for overall file opening process your parser provides a significant speedup:

Command Mean [s] Min [s] Max [s] Relative
cat samples.txt \| xargs node parse.mjs scoiatael 6.868 ± 0.086 6.727 7.014 1.14 ± 0.05
cat samples.txt \| xargs node parse.mjs pastelmind 6.049 ± 0.262 5.682 6.361 1.00

(where parse.mjs is here, Node is v18.2.0 and hardware is CPU: 11th Gen Intel i7-1185G7 (8) @ 4.800GHz)

@pastelmind Maybe the simplest way forward it to just merge this PR and then replace my implementation with yours?

scoiatael commented 2 years ago

@pastelmind had some spare time and managed to find pretty substantial optimizations; right now the results are:

Command Mean [s] Min [s] Max [s] Relative
cat samples.txt \| xargs node parse.mjs scoiatael 4.793 ± 0.076 4.677 4.899 1.00
cat samples.txt \| xargs node parse.mjs pastelmind 5.889 ± 0.052 5.815 5.977 1.23 ± 0.02
scoiatael commented 2 years ago

memoize results for each character instead of lookup

refers to STRING_TOKEN_JT - which pre-computes a boolean value for each character instead of doing two map lookups at runtime :) It was originally intended to be a specialized jump-table (hence JT suffix) but turned out that simple boolean array works best for this case.

It seems that avoiding unnecessary memory allocations greatly boosted your lexer's performance.

If you are referring to using Array instead of Generator then not really - I did it just to have clean demarkation line when benchmarking with 0x. This change can be easily reverted if we ever start to run into memory issues :)

PS. Hope you feel better now :)

scoiatael commented 2 years ago

@pastelmind are we waiting on someone / something? Dunno what the process is, but I can't merge it on my own ;)