Skip to content

Comments

Mark endian_scalar as unsafe.#6588

Merged
CasperN merged 4 commits intogoogle:masterfrom
CasperN:emplace
Apr 26, 2021
Merged

Mark endian_scalar as unsafe.#6588
CasperN merged 4 commits intogoogle:masterfrom
CasperN:emplace

Conversation

@CasperN
Copy link
Collaborator

@CasperN CasperN commented Apr 23, 2021

Also

  • removed the deprecated flexbuffer slice from example
  • fixed some cargo warnings

PTAL @krojew

This should fix #5825

I didn't add any // UNSAFE OK comments since the existing code is considered safe due to previous testing

Mark endian_scalar as unsafe.
7829fe1 
Also
- removed the deprecated flexbuffer slice from example
- fixed some cargo warnings
@github-actions github-actions bot added c++ codegen Involving generating code from schema rust labels Apr 23, 2021
@krojew
Copy link
Contributor

krojew commented Apr 24, 2021

What about read_scalar*?

@CasperN
Copy link
Collaborator Author

CasperN commented Apr 24, 2021

Btw, I also considered adding assertions instead of adding unsafe. We get a ~10% regression in some cases.

HEAD

test create_byte_vector_100_naive       ... bench:         719 ns/iter (+/- 56) = 139 MB/s
test create_byte_vector_100_optimal     ... bench:          24 ns/iter (+/- 3) = 4166 MB/s
test create_canonical_buffer_then_reset ... bench:         950 ns/iter (+/- 102) = 315 MB/s
test create_string_10                   ... bench:          26 ns/iter (+/- 3) = 384 MB/s
test create_string_100                  ... bench:          32 ns/iter (+/- 3) = 3093 MB/s
test hundred_maps                       ... bench:      60,096 ns/iter (+/- 4,983) = 152 MB/s
test hundred_maps_pooled                ... bench:      61,406 ns/iter (+/- 6,650) = 148 MB/s
test push_vec_u64_to_map                ... bench:      29,969 ns/iter (+/- 3,367) = 412 MB/s
test push_vec_u64_to_map_direct         ... bench:      14,889 ns/iter (+/- 4,667) = 830 MB/s
test push_vec_u64_to_map_direct_reused  ... bench:      12,101 ns/iter (+/- 936) = 1021 MB/s
test push_vec_u64_to_map_reused         ... bench:      26,675 ns/iter (+/- 2,637) = 463 MB/s
test push_vec_with_indirect             ... bench:      20,389 ns/iter (+/- 1,341) = 151 MB/s
test push_vec_without_indirect          ... bench:      21,637 ns/iter (+/- 2,597) = 426 MB/s
test read_monsters                      ... bench:     148,441 ns/iter (+/- 19,836) = 118 MB/s
test serialize_monsters                 ... bench:     168,858 ns/iter (+/- 22,882) = 103 MB/s
test traverse_canonical_buffer          ... bench:         116 ns/iter (+/- 18) = 2586 MB/s

With Assertions

test create_byte_vector_100_naive       ... bench:         858 ns/iter (+/- 78) = 116 MB/s
test create_byte_vector_100_optimal     ... bench:          28 ns/iter (+/- 4) = 3571 MB/s
test create_canonical_buffer_then_reset ... bench:       1,038 ns/iter (+/- 124) = 289 MB/s
test create_string_10                   ... bench:          28 ns/iter (+/- 3) = 357 MB/s
test create_string_100                  ... bench:          34 ns/iter (+/- 4) = 2911 MB/s
test hundred_maps                       ... bench:      63,305 ns/iter (+/- 5,542) = 144 MB/s
test hundred_maps_pooled                ... bench:      63,436 ns/iter (+/- 5,745) = 144 MB/s
test push_vec_u64_to_map                ... bench:      30,453 ns/iter (+/- 3,207) = 405 MB/s
test push_vec_u64_to_map_direct         ... bench:      14,934 ns/iter (+/- 952) = 827 MB/s
test push_vec_u64_to_map_direct_reused  ... bench:      12,369 ns/iter (+/- 1,576) = 999 MB/s
test push_vec_u64_to_map_reused         ... bench:      27,199 ns/iter (+/- 2,697) = 454 MB/s
test push_vec_with_indirect             ... bench:      20,701 ns/iter (+/- 1,693) = 149 MB/s
test push_vec_without_indirect          ... bench:      21,738 ns/iter (+/- 2,212) = 424 MB/s
test read_monsters                      ... bench:     150,519 ns/iter (+/- 16,903) = 116 MB/s
test serialize_monsters                 ... bench:     167,720 ns/iter (+/- 14,090) = 104 MB/s
test traverse_canonical_buffer          ... bench:         112 ns/iter (+/- 11) = 2678 MB/s

@CasperN
Copy link
Collaborator Author

CasperN commented Apr 24, 2021

 What about read_scalar*?

@krojew, oops, I should've noticed that

krojew
krojew reviewed Apr 25, 2021
View reviewed changes
rust/flatbuffers/src/endian_scalar.rs Outdated
/// Place an EndianScalar into the provided mutable byte slice. Performs
/// endian conversion, if necessary.
/// # Safety
/// Caller must ensure `s.len() > size_of::<T>()`.
Copy link
Contributor

@krojew krojew Apr 25, 2021

Choose a reason for hiding this comment

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

I think we should either list all safety requirements or none, e.g. no overlapping requirement is missing.

Copy link
Collaborator Author

@CasperN CasperN Apr 25, 2021

Choose a reason for hiding this comment

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

Done, I couldn't think of other requirements.

@krojew
Copy link
Contributor

krojew commented Apr 26, 2021

Looks good! We should add resolution info to rust advisory after a release.

@CasperN CasperN merged commit c24031c into google:master Apr 26, 2021
@CasperN CasperN deleted the emplace branch August 24, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@krojew krojew krojew left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ codegen Involving generating code from schema rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rust: read_scalar(_at) is unsound

2 participants

@CasperN @krojew