WIP: add benchmarking #192
Conversation
There was a problem hiding this comment.
benchmarks that involve IO are going to be especially difficult to interpret, since so much depends on hardware, read congestion, caching, etc etc. My inclination would be to just use the load_save example which does time those operations, which is useful enough for getting a loose sense of the impact of various changes?
If you want to do actual benchmarks, I think it is best to keep IO out of the benchmarks: load some files into memory first, and then benchmark the parsing.
I think it would also be useful to try and keep these benchmarks narrow in scope, e.g. benchmark reading just some .glif files, and benchmark just parsing font_info.plist, etc?
| pub fn load( | ||
| base_dir: &Path, | ||
| glyph_names: &NameList, | ||
| request: &crate::DataRequest, |
There was a problem hiding this comment.
I would not include DataRequest as part of the base API here, especially if it is only being used to thread through whether or not we load the glyph libs?
There was a problem hiding this comment.
Oh right, could have made it a bool.
There was a problem hiding this comment.
Or make it a separate method or something? I'd prefer not to break the public API for this (and we may not need to merge this at all, it sounds like?)
| @@ -13,6 +13,7 @@ | |||
| pub struct DataRequest { | |||
| pub layers: bool, | |||
| pub lib: bool, | |||
| pub glyph_lib: bool, | |||
There was a problem hiding this comment.
Is this a useful addition, or is it only useful in the context of benchmarking?
There was a problem hiding this comment.
This was primarily added to test the impact of glyph lib loading on loading times. Result: it roughly doubles parsing time on my Linux and Windows machines. Ouch. I currently have no other use for it, but this PR is a WIP for a reason :)
Hm, yes. That's actually what I wanted to test here. I found my Windows work machine takes roughly double the time of my Linux PC :( I agree with the rest of your comment. I haven't fully thought through yet what I want to benchmark and why. |
|
This is very old, do you plan to finish it or could it be closed? |
I'm playing a bit with criterion, as per #177.
Needs https://github.com/madig/noto-amalgamated and the glyphs2ufo output for NotoSans-MM.glyphs.