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

Store a reference to environment in session to tie its lifetime #46

Merged
merged 1 commit into from
Dec 27, 2020

Conversation

nbigaouette
Copy link
Owner

@nbigaouette nbigaouette commented Dec 27, 2020

By storing a reference to the environment, a session cannot be built in a specific way that causes segmentation faults. This forces a let binding on the environment.

Unfortunately, the fix prevents the monomorphization trick employed for with_model_from_file() which had to be merged with with_model_from_file_monomorphized(). Since this should only be called once per program, I guess the cost is not that much.

Closes #42

@nbigaouette nbigaouette force-pushed the 42-tie-session-lifetime-to-environment branch from ae37d58 to 81c2d32 Compare December 27, 2020 00:19
@codecov-io
Copy link

codecov-io commented Dec 27, 2020

Codecov Report

Merging #46 (7198ab1) into master (4a02bc0) will decrease coverage by 5.04%.
The diff coverage is 21.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   19.18%   14.13%   -5.05%     
==========================================
  Files          17       18       +1     
  Lines         610      962     +352     
==========================================
+ Hits          117      136      +19     
- Misses        493      826     +333     
Impacted Files Coverage Δ
...runtime-sys/src/generated/macos/x86_64/bindings.rs 0.00% <ø> (ø)
onnxruntime/src/session.rs 0.00% <0.00%> (ø)
onnxruntime/src/lib.rs 32.53% <41.02%> (+7.53%) ⬆️
onnxruntime/src/environment.rs 89.83% <50.00%> (+8.47%) ⬆️
onnxruntime/src/memory.rs 93.33% <100.00%> (ø)
onnxruntime/src/tensor/ort_tensor.rs 82.35% <100.00%> (ø)

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 694bb7c...7198ab1. Read the comment docs.

@nbigaouette nbigaouette force-pushed the 42-tie-session-lifetime-to-environment branch from 81c2d32 to 7198ab1 Compare December 27, 2020 03:25
@nbigaouette nbigaouette merged commit 3b804d4 into master Dec 27, 2020
@nbigaouette nbigaouette deleted the 42-tie-session-lifetime-to-environment branch December 27, 2020 03:44
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.

Session is not tied by lifetime to the environment => easy segfaults
2 participants