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

Stop using Julia's size classes when using MMTk #31

Merged
merged 6 commits into from
Feb 27, 2024

Conversation

udesou
Copy link

@udesou udesou commented Oct 5, 2023

When allocating an object, we are still allocating the number of bytes according to Julia's size classes, which may be an overestimation of the number of bytes an object actually requires. This PR removes this since this is specific to Julia's stock GC.

@udesou udesou marked this pull request as ready for review October 5, 2023 09:22
@udesou udesou requested a review from qinsoon October 5, 2023 09:22
src/mmtk-gc.c Show resolved Hide resolved
src/mmtk-gc.c Outdated Show resolved Hide resolved
@qinsoon
Copy link
Member

qinsoon commented Oct 6, 2023

This PR aligns any object size to 16 bytes. As the smallest size class is 8 bytes, we may allocate more memory than before, and could cause slowdown. MMTk assumes the allocation size is a multiple of VM::MIN_ALIGNMENT, which is defined as 4 for Julia (https://github.com/mmtk/mmtk-julia/blob/baeffd2d4e809289ecd962795142dd02bf59f8a7/mmtk/src/lib.rs#L43). Aligning to 4 bytes should be enough for MMTk, and aligning to 16 bytes may be unnecessary unless Julia requires it.

@udesou
Copy link
Author

udesou commented Oct 6, 2023

This PR aligns any object size to 16 bytes. As the smallest size class is 8 bytes, we may allocate more memory than before, and could cause slowdown. MMTk assumes the allocation size is a multiple of VM::MIN_ALIGNMENT, which is defined as 4 for Julia (https://github.com/mmtk/mmtk-julia/blob/baeffd2d4e809289ecd962795142dd02bf59f8a7/mmtk/src/lib.rs#L43). Aligning to 4 bytes should be enough for MMTk, and aligning to 16 bytes may be unnecessary unless Julia requires it.

8-byte aligned classes are used for strings:

// the 8-byte aligned pools are only used for Strings
I think everything else is aligned to 16. I'll change the code to make sure I use that alignment only for strings.

@qinsoon
Copy link
Member

qinsoon commented Oct 6, 2023

This PR aligns any object size to 16 bytes. As the smallest size class is 8 bytes, we may allocate more memory than before, and could cause slowdown. MMTk assumes the allocation size is a multiple of VM::MIN_ALIGNMENT, which is defined as 4 for Julia (https://github.com/mmtk/mmtk-julia/blob/baeffd2d4e809289ecd962795142dd02bf59f8a7/mmtk/src/lib.rs#L43). Aligning to 4 bytes should be enough for MMTk, and aligning to 16 bytes may be unnecessary unless Julia requires it.

8-byte aligned classes are used for strings:

// the 8-byte aligned pools are only used for Strings

I think everything else is aligned to 16. I'll change the code to make sure I use that alignment only for strings.

I am actually a bit confused by the comments in the code above ("the 8-byte aligned pools are only used for Strings"). They have 8 bytes aligned pools all the way up to 136 bytes. Does that mean all the objects in that size range are strings? That doesn't sound right.

Edit: Nvm. I got it.

@qinsoon
Copy link
Member

qinsoon commented Oct 6, 2023

For the current code,

  • String size is aligned up to 16 bytes (but this will be fixed so string size is unchanged -- aligned to 8 bytes).
  • Objects between 16 bytes to 136 bytes should have the same allocation size as before.
  • Objects larger than 144 bytes may have a smaller size.

@udesou udesou requested a review from qinsoon October 6, 2023 03:29
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon
Copy link
Member

qinsoon commented Oct 6, 2023

Can you get performance results for the PR (comparing with master)?

@udesou
Copy link
Author

udesou commented Oct 6, 2023

Can you get performance results for the PR (comparing with master)?

Yes, I was doing it already, without considering the latest changes, but I'll re-run everything and put the results here once I get them.

@udesou udesou merged commit 9460689 into mmtk:master Feb 27, 2024
1 check passed
udesou added a commit to mmtk/mmtk-julia that referenced this pull request Feb 27, 2024
When allocating an object, we are still allocating the number of bytes
according to Julia's size classes, which may be an overestimation of the
number of bytes an object actually requires. This PR removes this since
this is specific to Julia's stock GC. This affects not only allocation
itself but needs to be reflected in the `get_current_size` function.

Needs to be merged with mmtk/julia#31.
mergify bot pushed a commit to mmtk/mmtk-julia that referenced this pull request Feb 27, 2024
When allocating an object, we are still allocating the number of bytes
according to Julia's size classes, which may be an overestimation of the
number of bytes an object actually requires. This PR removes this since
this is specific to Julia's stock GC. This affects not only allocation
itself but needs to be reflected in the `get_current_size` function.

Needs to be merged with mmtk/julia#31.

(cherry picked from commit efb9e10)

# Conflicts:
#	mmtk/Cargo.toml
udesou added a commit to udesou/julia that referenced this pull request Feb 28, 2024
Stop using Julia's size classes when using MMTk
qinsoon pushed a commit to qinsoon/julia that referenced this pull request May 2, 2024
…liaLang#51237)

Stdlib: NetworkOptions
URL: https://github.com/JuliaLang/NetworkOptions.jl.git
Stdlib branch: master
Julia branch: master
Old commit: 976e51a
New commit: aab83e5
Julia version: 1.11.0-DEV
NetworkOptions version: 1.2.0(Does not match)
Bump invoked by: @DilumAluthge
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/NetworkOptions.jl@976e51a...aab83e5

```
$ git log --oneline 976e51a..aab83e5
aab83e5 Reset BUNDLED_KNOWN_HOSTS_FILE in case we serialized a value due to precompilation (mmtk#31)
```

Co-authored-by: Dilum Aluthge <[email protected]>
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