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

refactor: remove TypeInfo and StructInfo from StructRef #109

Merged
merged 3 commits into from
Apr 9, 2020

Conversation

Wodann
Copy link
Collaborator

@Wodann Wodann commented Mar 31, 2020

Instead we collect the most recent TypeInfo pointer from the Runtime's
GcRuntime. This allows us to maintain the StructRef even after hot
reloads.

@Wodann Wodann requested a review from baszalmstra March 31, 2020 14:01
@Wodann Wodann self-assigned this Mar 31, 2020
Comment on lines 179 to 186
let runtime_ref = self.runtime.borrow();
let type_info = unsafe { &*runtime_ref.gc().ptr_type(self.handle.handle()).inner() };
type_info.name()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This in particular worries me, because there is a significant performance overhead to retrieving a struct's type information. This change affects StructRef::get, StructRef::replace, and invoke_fn!.

It would already be a lot better if we didn't still have to borrow the Runtime. That means passing the &Runtime as an additional parameter to the ArgumentReflection trait's type_name and type_guid functions, though.

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #109 into master will increase coverage by 0.61%.
The diff coverage is 85.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   78.25%   78.87%   +0.61%     
==========================================
  Files         141      141              
  Lines        8949     8724     -225     
==========================================
- Hits         7003     6881     -122     
+ Misses       1946     1843     -103     
Impacted Files Coverage Δ
crates/mun_abi/src/autogen.rs 64.61% <ø> (-0.05%) ⬇️
crates/mun_runtime/src/reflection.rs 55.31% <63.63%> (+1.61%) ⬆️
crates/mun_runtime/src/struct.rs 84.11% <84.31%> (-3.80%) ⬇️
crates/mun_abi/src/autogen_impl.rs 98.75% <85.71%> (+0.62%) ⬆️
crates/mun_codegen/src/ir/abi_types.rs 100.00% <100.00%> (ø)
crates/mun_codegen/src/ir/type_table.rs 84.96% <100.00%> (+2.02%) ⬆️
crates/mun_runtime/src/garbage_collector.rs 100.00% <100.00%> (ø)
crates/mun_runtime/src/lib.rs 83.54% <100.00%> (-2.67%) ⬇️
crates/mun_runtime/src/macros.rs 64.91% <100.00%> (+0.62%) ⬆️
crates/mun_runtime/src/marshal.rs 100.00% <100.00%> (ø)
... and 49 more

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 d9b42cd...69ee4e2. Read the comment docs.

@Wodann Wodann force-pushed the refactor/struct-ref branch from 28e2057 to 8a1c25c Compare April 5, 2020 10:42
@Wodann
Copy link
Collaborator Author

Wodann commented Apr 5, 2020

@baszalmstra I pushed a fix for the discussion about the runtime overhead of borrowing a Runtime and rebased on master.

Wodann added 3 commits April 8, 2020 17:19
Instead we collect the most recent TypeInfo pointer from the Runtime's
GcRuntime. This allows us to maintain the StructRef even after hot
reloads
This prevents duplicate storage as the struct's name is already stored
in the TypeInfo
@Wodann Wodann force-pushed the refactor/struct-ref branch from 8a1c25c to 69ee4e2 Compare April 8, 2020 15:26
@Wodann Wodann merged commit 105755c into mun-lang:master Apr 9, 2020
@Wodann Wodann added this to the Mun v0.2 milestone May 14, 2020
@Wodann Wodann deleted the refactor/struct-ref branch May 14, 2020 15:10
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.

2 participants