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.

Sign up for GitHub

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

Jump to bottom

Add a pre-parsed FieldPath for faster repeated uses of GetPath calls #4081

Closed

james7132 wants to merge 21 commits into bevyengine:main from james7132:parse-path

Member

james7132 commented Mar 2, 2022 •

edited

Loading

Objective

Fixes #4080. Implement a pre-parsed version of GetPath functions.

Solution

Implement FieldPath a struct that parses a field path ahead of time and caches the sequence of field accesses to avoid string parsing when fetching a field. Added a few additional changes:

Clean up some of the duplicated code in that file.
Extended the string format to allow for specifying normal struct field indexes instead of normal names. This can be faster to use for FieldPath lookups, but it can be much more fragile when serialized to assets. Can be used to transform string lookups into index lookups static type information.

Future Directions

If we get non-reference bound type information, we can further optimize this by looking up field indexes during parsing using a TypeRegistry.

james7132 added 2 commits

March 2, 2022 05:37


          Refactor GetPath's parser

935ac62


          Add FieldPath and Access

6c10c54

github-actions bot added the S-Needs-Triage label

james7132 added A-Animation A-Reflection C-Feature and removed S-Needs-Triage labels


          Darn clippy

629d3b1

james7132 added the C-Performance label

james7132 changed the title ~~Parse path~~ Add FieldPath, a pre-parsed field path for faster repeated uses of GetPath

james7132 changed the title ~~Add FieldPath, a pre-parsed field path for faster repeated uses of GetPath~~ Add a pre-parsed FieldPath for faster repeated uses of GetPath calls

james7132 added 8 commits

March 2, 2022 06:36


          remove spurious debug prints

b972af8


          Clean up code and add doc comments

f1a09aa


          Clippy

6e7a5db


          Cleanup code a bit more

f34899f


          Handle field number parsing

bc421c0


          Update path tests

275d8e0


          Add a Display implementation for FieldPath

2032d88


          Add documentation for field index path s

837d06c

MrGVSV suggested changes

View reviewed changes

Member

MrGVSV left a comment

Looks good overall. I think it would nice to add some tests for FieldPath, though, to make sure it works properly (especially after multiple uses) in the future.

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

crates/bevy_reflect/src/path.rs Show resolved Hide resolved

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

james7132 and others added 3 commits

March 7, 2022 21:24


          Clarify doc comment on field index paths.

5ce165b

Co-authored-by: MrGVSV <[email protected]>


          impl -> dyn

587757a


          Add tests

3ced0de

MrGVSV reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs Show resolved Hide resolved

MrGVSV reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs Show resolved Hide resolved

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

crates/bevy_reflect/src/path.rs Show resolved Hide resolved

alice-i-cecile added C-Code-Quality and removed C-Feature labels

MrGVSV approved these changes

View reviewed changes

alice-i-cecile reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs

+                      Ok(current)
+                  }
+                  pub fn get_field<'r, 'p, T: Reflect>(

Member

alice-i-cecile May 16, 2022

This needs docs.

Member

alice-i-cecile May 16, 2022

Externally, it's very hard to tell the difference between the field method and the get_field. I would have expected this to be the fallible version based on the name, but they both return results and have the same type signature.

@DJMcNab comments that get_field appears to do automatic downcasting.

The naming scheme needs to be fixed.

Member Author

james7132 May 26, 2022

This mirrors the same API for GetPath, if we update it here, we should update it in both places. The main difference is that one has a concrete type and the other returns a dyn Reflect reference.

alice-i-cecile reviewed

View reviewed changes

crates/bevy_reflect/src/path.rs

+                      })
+                  }
+                  pub fn get_field_mut<'r, 'p, T: Reflect>(

Member

alice-i-cecile May 16, 2022

Docs.

alice-i-cecile requested changes

View reviewed changes

Member

alice-i-cecile left a comment

I like the direction here, but have some docs and code quality nits that should be addressed.

crates/bevy_reflect/src/path.rs

+                      Ok(current)
+                  }
+                  pub fn get_field<'r, 'p, T: Reflect>(

Member

alice-i-cecile May 16, 2022

Externally, it's very hard to tell the difference between the field method and the get_field. I would have expected this to be the fallible version based on the name, but they both return results and have the same type signature.

@DJMcNab comments that get_field appears to do automatic downcasting.

The naming scheme needs to be fixed.

crates/bevy_reflect/src/path.rs

-                                      });
+              impl fmt::Display for FieldPath {
+                  fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+                      for (idx, (access, _)) in self.0.iter().enumerate() {

Member

alice-i-cecile May 16, 2022

This should be pulled out into a utility function and tested IMO.

crates/bevy_reflect/src/path.rs Show resolved Hide resolved

crates/bevy_reflect/src/path.rs Show resolved Hide resolved

crates/bevy_reflect/src/path.rs Outdated Show resolved Hide resolved

james7132 and others added 7 commits

May 16, 2022 12:06


          Update docs.

e607e91

Co-authored-by: Alice Cecile <[email protected]>


          Update docs.

fc3cbf3

Co-authored-by: Alice Cecile <[email protected]>


          Update docs.

8933df4

Co-authored-by: Alice Cecile <[email protected]>


          Update docs.

18b217b

Co-authored-by: Alice Cecile <[email protected]>


          Use associated constants

c2a027a


          Add some doc strings

e58ee2d


          Use the constants more

47194e0

This was referenced Jun 9, 2022

[Merged by Bors] - bevy_reflect: Add statically available type info for reflected types #4042

Closed

Low level Component Reflection utiliites for Animation #4985

Closed

Weibye added the S-Adopt-Me label

alice-i-cecile requested a review from MrGVSV

January 11, 2023 17:44

alice-i-cecile approved these changes

View reviewed changes

Member

alice-i-cecile left a comment

I would still like more docs, but I won't block on them.

Member

alice-i-cecile commented Jan 11, 2023

@james7132 @MrGVSV now that #6560 is merged, we should clean this up and merge it as James pointed out there.

Member

MrGVSV commented Jan 12, 2023

bors try

Contributor

bors bot commented Jan 12, 2023

try

Merge conflict.

Member

MrGVSV commented Jan 12, 2023 •

edited

Loading

@james7132 if you're too busy to do so, I can adopt this PR, fix the merge conflicts, and address any remaining issues/reviews. Just let me know! :)

Member Author

james7132 commented Jan 12, 2023

If you could ,that'd be great. My focus has been elsewhere.

MrGVSV mentioned this pull request

[Merged by Bors] - bevy_reflect: Pre-parsed paths #7321

Closed

2 tasks

Member

MrGVSV commented Jan 21, 2023

Closing this as it's now been adopted by #7321.

MrGVSV closed this

bors bot pushed a commit that referenced this pull request


          bevy_reflect: Pre-parsed paths (#7321)

8cd59b6

# Objective

> ℹ️ **This is an adoption of #4081 by @james7132**

Fixes #4080.

Provide a way to pre-parse reflection paths so as to avoid having to parse at each call to `GetPath::path` (or similar method).

## Solution

Adds the `ParsedPath` struct (named `FieldPath` in the original PR) that parses and caches the sequence of accesses to a reflected element. This is functionally similar to the `GetPath` trait, but removes the need to parse an unchanged path more than once.

### Additional Changes

Included in this PR from the original is cleaner code as well as the introduction of a new pathing operation: field access by index. This allows struct and struct variant fields to be accessed in a more performant (albeit more fragile) way if needed. This operation is faster due to not having to perform string matching. As an example, if we wanted the third field on a struct, we'd write `#2`—where `#` denotes indexed access and `2` denotes the desired field index.

This PR also contains improved documentation for `GetPath` and friends, including renaming some of the methods to be more clear to the end-user with a reduced risk of getting them mixed up.

### Future Work

There are a few things that could be done as a separate PR (order doesn't matter— they could be followup PRs or done in parallel). These are:

- [x] ~~Add support for `Tuple`. Currently, we hint that they work but they do not.~~ See #7324
- [ ] Cleanup `ReflectPathError`. I think it would be nicer to give `ReflectPathError` two variants: `ReflectPathError::ParseError` and `ReflectPathError::AccessError`, with all current variants placed within one of those two. It's not obvious when one might expect to receive one type of error over the other, so we can help by explicitly categorizing them.

---

## Changelog

- Cleaned up `GetPath` logic
- Added `ParsedPath` for cached reflection paths
- Added new reflection path syntax: struct field access by index (example syntax: `foo#1`)
- Renamed methods on `GetPath`:
  - `path` -> `reflect_path`
  - `path_mut` -> `reflect_path_mut`
  - `get_path` -> `path`
  - `get_path_mut` -> `path_mut`

## Migration Guide

`GetPath` methods have been renamed according to the following:
- `path` -> `reflect_path`
- `path_mut` -> `reflect_path_mut`
- `get_path` -> `path`
- `get_path_mut` -> `path_mut`


Co-authored-by: Gino Valente <[email protected]>

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request


          bevy_reflect: Pre-parsed paths (bevyengine#7321)

69d5d7e

# Objective

> ℹ️ **This is an adoption of bevyengine#4081 by @james7132**

Fixes bevyengine#4080.

Provide a way to pre-parse reflection paths so as to avoid having to parse at each call to `GetPath::path` (or similar method).

## Solution

Adds the `ParsedPath` struct (named `FieldPath` in the original PR) that parses and caches the sequence of accesses to a reflected element. This is functionally similar to the `GetPath` trait, but removes the need to parse an unchanged path more than once.

### Additional Changes

Included in this PR from the original is cleaner code as well as the introduction of a new pathing operation: field access by index. This allows struct and struct variant fields to be accessed in a more performant (albeit more fragile) way if needed. This operation is faster due to not having to perform string matching. As an example, if we wanted the third field on a struct, we'd write `bevyengine#2`—where `#` denotes indexed access and `2` denotes the desired field index.

This PR also contains improved documentation for `GetPath` and friends, including renaming some of the methods to be more clear to the end-user with a reduced risk of getting them mixed up.

### Future Work

There are a few things that could be done as a separate PR (order doesn't matter— they could be followup PRs or done in parallel). These are:

- [x] ~~Add support for `Tuple`. Currently, we hint that they work but they do not.~~ See bevyengine#7324
- [ ] Cleanup `ReflectPathError`. I think it would be nicer to give `ReflectPathError` two variants: `ReflectPathError::ParseError` and `ReflectPathError::AccessError`, with all current variants placed within one of those two. It's not obvious when one might expect to receive one type of error over the other, so we can help by explicitly categorizing them.

---

## Changelog

- Cleaned up `GetPath` logic
- Added `ParsedPath` for cached reflection paths
- Added new reflection path syntax: struct field access by index (example syntax: `foo#1`)
- Renamed methods on `GetPath`:
  - `path` -> `reflect_path`
  - `path_mut` -> `reflect_path_mut`
  - `get_path` -> `path`
  - `get_path_mut` -> `path_mut`

## Migration Guide

`GetPath` methods have been renamed according to the following:
- `path` -> `reflect_path`
- `path_mut` -> `reflect_path_mut`
- `get_path` -> `path`
- `get_path_mut` -> `path_mut`


Co-authored-by: Gino Valente <[email protected]>

james7132 deleted the parse-path branch

March 14, 2023 04:15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Animation A-Reflection C-Code-Quality C-Performance S-Adopt-Me