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

Post-release dependency version bump for v0.21.0 #1013

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

wks
Copy link
Collaborator

@wks wks commented Nov 3, 2023

Bump the version of dependencies to their latest version.

The "atomic" crate, since version 0.6.0, requires the T in Atomic<T> to implement bytemuck::NoUnint. It is a marker trait for types that satisfies some requirements. One requirement is that T must not have any padding bytes (in the middle or at the end). The derive macro #[derive(NoUninit)] can be used on custom types to automatically check if the type satisfies its requirements. Two notable types are:

  1. enum MMapStrategy. It is missing a representation annotation which NoUninit requires. This PR adds #[repr(u8)] to fix this.
  2. WORKER_ORDINAL: Atomic<Option<ThreadId>>. The Option type does have padding bytes, making it un-eligible for Atomic<T>. This PR changes it to Atomic<ThreadId>, with ThreadId::max representing the uninitialized value.

@wks wks added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Nov 3, 2023
@wks wks force-pushed the release/post-release-bump-v0.21.0 branch from 39eaeda to bd46bef Compare November 13, 2023 03:20
Bump the version of dependencies to their latest version.

The "atomic" crate, since version 0.6.0, requires the `T` in `Atomic<T>`
to implement `bytemuck::NoUnint`.  It is a marker trait for types that
satisfies some requirements.  One requirement is that `T` must not have
any padding bytes (in the middle or at the end).  The derive macro
`#[derive(NoUninit)]` can be used on custom types to automatically check
if the type satisfies its requirements.  Two notable types are:

1.  `enum MMapStrategy`.  It is missing a representation annotation
    which `NoUninit` requires.  This PR adds `#[repr(u8)]` to fix this.
2.  `WORKER_ORDINAL: Atomic<Option<ThreadId>>`.  The `Option` type does
    have padding bytes, making it un-eligible for `Atomic<T>`.  This PR
    changes it to `Atomic<ThreadId>`, with `ThreadId::max` representing
    the uninitialized value.
@wks wks force-pushed the release/post-release-bump-v0.21.0 branch from bd46bef to 681dabd Compare November 27, 2023 05:45
@wks
Copy link
Collaborator Author

wks commented Nov 27, 2023

binding-refs
OPENJDK_BINDING_REPO=wks/mmtk-openjdk
OPENJDK_BINDING_REF=release/post-release-bump-v0.21.0

Copy link
Contributor

@udesou udesou left a comment

Choose a reason for hiding this comment

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

LGTM.

@wks wks added this pull request to the merge queue Dec 4, 2023
Merged via the queue into mmtk:master with commit 1ac23e7 Dec 4, 2023
19 of 20 checks passed
@wks wks deleted the release/post-release-bump-v0.21.0 branch December 4, 2023 04:49
@k-sareen
Copy link
Collaborator

k-sareen commented Dec 4, 2023

@wks The performance regression CI seems to be broken. Something about git dependency version.

@qinsoon
Copy link
Member

qinsoon commented Dec 4, 2023

@wks The performance regression CI seems to be broken. Something about git dependency version.

Maybe try manually merge mmtk/mmtk-openjdk#259 first. Then rerun the failed CI jobs.

The OpenJDK PR was not automatically merged due to an error here: https://github.com/mmtk/mmtk-core/actions/runs/7082168787/job/19272539610. The PR uses branch for the mmtk dep, and our script does not handle this case.

# We assume the file already includes `git` and `rev`. We just update them. This will preserve the
# original formatting and comments.

@wks
Copy link
Collaborator Author

wks commented Dec 4, 2023

Interestingly the mmtk-core PR is merged, but the mmtk-openjdk PR is not. I'll update and manually merge the mmtk-openjdk PR.

@qinsoon
Copy link
Member

qinsoon commented Dec 4, 2023

Interestingly the mmtk-core PR is merged, but the mmtk-openjdk PR is not. I'll update and manually merge the mmtk-openjdk PR.

The reason is what I wrote above. However, even if the mmtk-openjdk PR is merged as expected, we will still see failed jobs for mmtk-core perf CI, as those jobs are executed before the binding PRs are merged. I don't know if what would be a good way to deal with it (maybe we should wait until the binding PRs are merged and then execute the perf CI?)

@wks
Copy link
Collaborator Author

wks commented Dec 4, 2023

The reason is what I wrote above. However, even if the mmtk-openjdk PR is merged as expected, we will still see failed jobs for mmtk-core perf CI, as those jobs are executed before the binding PRs are merged. I don't know if what would be a good way to deal with it (maybe we should wait until the binding PRs are merged and then execute the perf CI?)

Yes. I think we should re-run the perf CI once the binding PR is merged. PRs may fail to merge for one reason or another, and there should be a way to manually trigger those things.

@wks
Copy link
Collaborator Author

wks commented Dec 4, 2023

I restarted the performance regression CI after merging the mmtk-openjdk PR https://github.com/mmtk/mmtk-core/actions/runs/7083003792

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants