Skip to content

Use NonNull pointer in heap version#298

Merged
bors-servo merged 1 commit intoservo:masterfrom
AngelicosPhosphoros:use_non_null
Jun 15, 2023
Merged

Use NonNull pointer in heap version#298
bors-servo merged 1 commit intoservo:masterfrom
AngelicosPhosphoros:use_non_null

Conversation

@AngelicosPhosphoros
Copy link

@AngelicosPhosphoros AngelicosPhosphoros commented Jun 3, 2023 •
edited
Loading

Since SmallVec allocate on heap only if capacity is larger than inner capacity and therefore larger than 1, it cannot be null. Also, this is same behaviour as std::vec::Vec which always uses non-null pointer, which is dangling if it has zero capacity.

This change reduces size of enum in some cases because it can exploit niche optimization. For example, size_of::<SmallVec<[u8; 8]>> is 24 bytes with changes from this PR while before it was 32 bytes.

Also:

  1. Changed some internal APIs to indicate that we work with non-nullable pointers.
  2. Updated natvis and fixed bug with incorrect calculation of is_inline intrinsic.

@AngelicosPhosphoros
Copy link
Author

AngelicosPhosphoros commented Jun 3, 2023 •
edited
Loading

Also, I think this changes make union feature useless because it would not provide size benefits anymore.

Another thing is capacity or size field in small vec implementation where field acts as length in inline version and as capacity in heap version. I don't see benefits of this compared to storing length directly and storing capacity of allocation in heap version.
On the other hand, storing capacity in heap version allow to store NonZeroUsize there and get size of Option<SmallVec<_>> smaller when used array is small.

Btw, it seems that CI is broken probably because latest version of Ubuntu doesn't have some packages used in CI.

@mbrubeck
Copy link
Collaborator

mbrubeck commented Jun 4, 2023

Hi, thanks for this work! Interestingly, I tried doing this before but reverted it in 0e8f5e9 because I couldn’t see any size improvements. I’m not sure if something was wrong with my implementation, or if I just made a mistake testing it.

The CI errors should be fixed by #299.

@AngelicosPhosphoros
Use NonNull pointer in heap version
9d96d7f 
Since `SmallVec` allocate on heap only if capacity is larger than inner
capacity and therefore larger than 1, it cannot be null.
Also, this is same behaviour as `std::vec::Vec` which always uses non-null pointer,
which is dangling if it has zero capacity.

This change reduces size of enum in some cases because it can exploit [niche optimization](https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#niche).
For example, `size_of::<SmallVec<[u8; 8]>>` is 24 bytes with changes from this PR while before it was 32 bytes.

Also:
1. Changed some internal APIs to indicate that we work with non-nullable pointers.
2. Updated natvis and fixed bug with incorrect calculation of `is_inline` intrinsic.
@AngelicosPhosphoros
Copy link
Author

AngelicosPhosphoros commented Jun 13, 2023

@mbrubeck
I fixed natvis for enum version.

And as I tested, natvis for union version didn't work in first place because windbg cannot read inline version for some reason. It writes "expected a member name" in watched variables, for example.

I also added is_inline to short debug representation because I think it is useful information.

@mbrubeck
Copy link
Collaborator

mbrubeck commented Jun 13, 2023

Thanks, this looks good! I'm going to give it one more careful review before merging it, in the next few days.

@mbrubeck
Copy link
Collaborator

mbrubeck commented Jun 15, 2023

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jun 15, 2023

📌 Commit 9d96d7f has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Jun 15, 2023

⌛ Testing commit 9d96d7f with merge 0c56f1b...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 15, 2023

☀️ Test successful - checks-github
Approved by: mbrubeck
Pushing 0c56f1b to master...

1 similar comment
@bors-servo
Copy link
Contributor

bors-servo commented Jun 15, 2023

☀️ Test successful - checks-github
Approved by: mbrubeck
Pushing 0c56f1b to master...

@bors-servo bors-servo merged commit 0c56f1b into servo:master Jun 15, 2023
This was referenced Jun 15, 2023
Merged
Closed
Closed
@AngelicosPhosphoros AngelicosPhosphoros deleted the use_non_null branch June 15, 2023 21:50
@mbrubeck mbrubeck mentioned this pull request Jun 17, 2023
Merged
@sffc
Copy link

sffc commented Aug 17, 2023

I suspect that this PR increases the code size of SmallVec::try_reserve, which is used by a number of entrypoints including SmallVec::insert_from_slice. On smallvec 1.10, with link-time optimization, insert_from_slice gets inlined to the call site and only ever calls down to rust_alloc. However, on smallvec 1.11, the function is no longer inlined and it calls rust_alloc, rust_realloc, and rust_dealloc. This causes about 1 KB more code to be linked into WASM than was previously required when using smallvec. I'm not 100% sure this is the culprit PR but it seems to be the only one between 1.10 and 1.11 touching this code path.

CC @Manishearth @robertbastian

@sffc sffc mentioned this pull request Aug 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbrubeck mbrubeck mbrubeck approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AngelicosPhosphoros @mbrubeck @bors-servo @sffc