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: Use std::string_view for prop conversions #43218

Closed
wants to merge 2 commits into from

Conversation

Saadnajmi
Copy link
Contributor

Summary:

As pointed out by @tido64 in one of my other PRs, we can be more efficient on prop conversions by using std::string_view instead of just casting to std::string, which may cause a copy. I thought I might as well do a Find+Replace for the whole codebase and see what breaks.

Changelog:

[GENERAL] [CHANGED] - Use std::string_view for prop conversions

Test Plan:

CI should pass

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 27, 2024
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,931,480 -8,206
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,289,773 -8,208
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 57ed0fb
Branch: main

Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

Hey @Saadnajmi! These all seem like instances of the same, sadly invalid, pattern - see my comment on the first one.

@@ -321,7 +321,7 @@ namespace facebook::react {
enum class EnumPropNativeComponentViewAlignment { Top, Center, BottomRight };

static inline void fromRawValue(const PropsParserContext& context, const RawValue &value, EnumPropNativeComponentViewAlignment &result) {
auto string = (std::string)value;
auto string = std::string_view{(std::string)value};
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting to string and then to string_view doesn't really make sense, I'm afraid. You're creating a temporary string (incurring the same copy as before btw), then effectively creating a dangling pointer to its contents. See proof by godbolt + asan.

I don't have enough context, but it might make sense to create a non-copying const string& / string_value accessor on RawValue, if we know the string is stored in some stable location that is safe to access for the life of our value reference.

Copy link
Collaborator

@tido64 tido64 Feb 28, 2024

Choose a reason for hiding this comment

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

Ah, that makes sense. I had assumed we were casting from an any-ish type and the conversion was "free". std::string_view accepts raw char*, would it be enough to drop the cast i.e., auto string = string_view{value};?

Edit: Nvm, it looks like the string conversion is handled by the underlying folly::dynamic instance. Implementing a RawValue::string_view method using its dynamic::c_str() seems possible? Sorry, on mobile so it's hard to do proper investigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL I learned about compiler explorer, pretty awesome!
OK, I'll keep this PR up a bit to explore an alternative path, but noted the current version is not what we want.

@Saadnajmi
Copy link
Contributor Author

Closing as I don't expect to come back to this

@Saadnajmi Saadnajmi closed this May 3, 2024
@Saadnajmi Saadnajmi deleted the string_view branch May 3, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants