go-openapi / spec

openapi specification object model
Apache License 2.0
394 stars 100 forks source link

Reduce memory usage at package init time #104

Closed fredbi closed 3 years ago

fredbi commented 5 years ago

The point with this PR is to avoid unnecessary memory requirements on packages importing this package because of some dependency but do not actually use its full functionality.

Most memory initialization is required by the expander so let’s allocate it only when required.

Signed-off-by: Frederic BIDON fredbi@yahoo.com

codecov[bot] commented 5 years ago

Codecov Report

Merging #104 (a26b582) into master (3f11504) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #104   +/-   ##
=======================================
  Coverage   58.76%   58.76%           
=======================================
  Files          25       25           
  Lines        1928     1928           
=======================================
  Hits         1133     1133           
  Misses        614      614           
  Partials      181      181           
Impacted Files Coverage Δ
operation.go 91.46% <ø> (ø)
spec.go 45.45% <ø> (-4.55%) :arrow_down:
cache.go 100.00% <100.00%> (ø)
expander.go 71.26% <100.00%> (+0.08%) :arrow_up:
schema_loader.go 89.23% <100.00%> (+0.08%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3f11504...a26b582. Read the comment docs.

casualjim commented 5 years ago

The state still exists, only now you have subtle races

fredbi commented 5 years ago

@casualjim I think you misunderstood my point. This is not about removing the state (that might be for another time), but about removing the memory allocated in init(). I have some packages using some go-openapi stuff depending on spec and which never attempt to call its API. When profiling, I've seen this memory chunk allocated just to store the schemas which will never get used. I am failing to see which races you are referring to. Maybe the PathLoader removal was going a bit too far (e.g. if a caller overrides this in its own init()?).

fredbi commented 5 years ago

@casualjim I've been thinking quite some time ago about removing the state altogether. If we want to do that (e.g. to run parallel tests in some other repos...), I believe this is about replacing these by a shallow clone: https://github.com/go-openapi/spec/blob/744796356cda816d3166723c28d45c5f58f590df/schema_loader.go#L257 and https://github.com/go-openapi/spec/blob/744796356cda816d3166723c28d45c5f58f590df/expander.go#L213

casualjim commented 5 years ago

That's a different story, but in that case, this PR doesn't address that because then there can't be any non-overridable globals anywhere. And it's likely that those tests will need to create their own

fredbi commented 5 years ago

@casualjim Indeed this is a different story, and that's why I intended to move forward in 2 steps: first was to allocate the cache seed lazily, and maybe second, remove the necessity for a state altogether. Again, thinking twice about that, I think I can revert the change on PathLoader, which is negligible anyway. How about reopening the present PR with that goal in mind?