API migration guide. #1133

wks · 2024-05-06T09:43:50Z

An API Migration Guide section is added to the porting guide.

A symbolic link is created as mmtk-core/API-MIGRATION.md which can be easily discovered from the root of the repository.

An *API Migration Guide* section is added to the porting guide. A symbolic link is created as `mmtk-core/API-MIGRATION.md` which can be easily discovered from the root of the repository.

qinsoon

I made some changes to the PR:

Add a CI check to make sure migration guide is updated if we have API changes.
Remove the symbolic link (symbolic links don't work for OS like windows). Instead, refer to the migration guide in README.
Move migration guide to a folder so we may extend it and split it based on versions.

I proposed using a more concise and more organised format for the migration guide.

qinsoon · 2024-05-07T00:55:11Z

docs/userguide/src/migration/prefix.md

+## 0.25.0
+
+
+### `ObjectReference` is no longer nullable


We would expect most people to just bump the MMTk version and attempt to fix on their own. When they struggle to do that, the migration guide needs to help them locate the issues they are facing. We should expect people to glance through the list and find related information for them. Thus I suggest using a more concise and more organised format.

I propose something like below:

### `ObjectReference` is no longer nullable: https://github.com/mmtk/mmtk-core/pull/1064 * Affected: everyone * Expected changes: * `ObjectReference` cannot refer to a null reference. * Use `Option<ObjectReference>` (Rust) or `mmtk::util::api_util::NullableObjectReference` (for native API). * Example migration: https://github.com/mmtk/mmtk-openjdk/pull/265 ### Write barriers use `Option<ObjectReference>` for `target`: https://github.com/mmtk/mmtk-core/pull/1130 * Affected: bindings using generational plans * Expected changes: * Bindings should properly pass `target: Option<ObjectRefernece>` for `object_reference_write_pre/post`. * `object_reference_write` is marked as deprecated and will be redesigned. Use `object_reference_write_pre/post` instead, for now. * Example: https://github.com/mmtk/mmtk-openjdk/pull/273 ### Instance methods of `ObjectReference` changed: https://github.com/mmtk/mmtk-core/pull/1122 * Affected: everyone * Expected changes: * Some methods in `ObjectReference` expect a type parameter `<VM: VMBinding>`. * Example: https://github.com/mmtk/mmtk-openjdk/pull/ ### The GC controller (a.k.a. coordinator) is removed: https://github.com/mmtk/mmtk-core/pull/1067 * Affected: everyone * Expected changes: * Code related with the GC controller is removed, and the bindings no longer need to spawn and run a GC controller. * Example: https://github.com/mmtk/mmtk-openjdk/pull/268

When the binding developers find an API changed, they want to know why this change is made, and more importantly, how they should change their existing usage. The problem with this format is that it is overly concise. For example, if we mention "Edge::load() now returns Option<ObjectReference>", the reader will think "What should it return after this change?" That's where the long explanation is needed.

But you are right. Most people expect to quickly locate the related information. I think we can add the "expected changes" list to the beginning of each item as a concise TL;DR-style hint, while the reader can continue reading if they find relevant.

I prefer a list of changes in the migration guide, such as https://getbootstrap.com/docs/5.0/migration/. Besides, the links to PRs should include all the information which people can continue reading to know more details.

However, we can maintain a list with optional links to detailed description for a PR, like https://v3-migration.vuejs.org/breaking-changes/. e.g.

### `ObjectReference` is no longer nullable: https://github.com/mmtk/mmtk-core/pull/1064 * Affected: everyone * Details: See here (this is optional) * Expected changes: * `ObjectReference` cannot refer to a null reference. * Use `Option<ObjectReference>` (Rust) or `mmtk::util::api_util::NullableObjectReference` (for native API). * Example: https://github.com/mmtk/mmtk-openjdk/pull/265

For example, if we mention "Edge::load() now returns Option<ObjectReference>", the reader will think "What should it return after this change?" That's where the long explanation is needed.

We state that ObjectReference cannot be null any more, ad Option<ObjectReference> should be used. I think this should be enough for users to know how to deal with a return value with Option<ObjectReference>. Plus, we have updated comments for the function Edge::load() for details.

I made another version in the bullet points style: https://github.com/wks/mmtk-core/blob/0db7a487a28d54a22949a81033878d862fa11081/docs/userguide/src/migration/prefix.md

It does look clearer at first glance, but I feel it is not right. It looks good as a summary of API changes, but that's probably not what VM binding developers need. When a binding developer pulls the mmtk-core repository, the binding will fail to compile, and the compiler and the IDE will highlight functions that have problems. That will be the same set of functions we listed in the bullet points version. I don't expect the VM binding developers to read through the bullet points in the Migration Guide because the compile errors will tell them what needs to be changed. If the change is obvious, the binding developer will just make the change and fix the compilation error.

The problem is, what if the binding developer finds one of the functions no longer compiles, and but does not know how to update the old code to adapt to the change? They will eventually find the answer by reading the API docs of affected functions. But it is easier if they can go to the Migration Guide, and find detailed descriptions about the old behavior and the new behavior. The Migration Guide should tell the reader what to do if the binding wants to do this or that.

I find the migration guide of the sysinfo crate quite helpful, especially the following part:

A new System::refresh_memory_specifics method and a new MemoryRefreshKind type were added, allowing you to control whether you want both RAM and SWAP memories to be updated or only one of the two. This change was needed because getting SWAP information on Windows is very slow.

MMTk used RefreshKind::new().with_memory() in get_system_total_memory. After bumping the version, it no longer compiles. I figured out how to make the change by reading that paragraph which told me I should switch to MemoryRefreshKind and also decide if I want the RAM size or the swap size. Because MMTk wants the physical memory size, I changed it to `MemoryRefreshKind::new().with_ram().

The problem is, what if the binding developer finds one of the functions no longer compiles, and but does not know how to update the old code to adapt to the change?

Exactly. I think most developers won't go straight to the migration guide, but instead they just pull the new version and try to fix compilation errors. They will turn to the migration guide when they find that they cannot fix some issues themselves. Hopefully the migration guide can help them locate where the compilation error is from, and provide them a link to an example -- that should be enough for most cases. So I suggest the migration guide to be concise and organized so that they can easily locate the issue they are searching for, and they can find the solution with an example, or with detailed migration description and the related PR.

I made a third version https://github.com/wks/mmtk-core/blob/feature/api-change-log3/docs/userguide/src/migration/prefix.md

It still has the bullet points style, but is not as concise as the second version, and contains more explanation. For the convenience of navigation, it now organises the changes in a hierarchy of (1) type, trait or module, and then (2) method. It is probably only needed for big changes like removing ObjectReference.

wks · 2024-05-07T01:33:16Z

Add a CI check to make sure migration guide is updated if we have API changes.

This should give a warning instead of failing immediately. Some API changes add new methods (such as adding new APIs), while others are source compatible (such as adding a type parameter <T> which can be automatically inferred). Those changes don't need the binding to be changed.

qinsoon · 2024-05-07T01:49:09Z

Add a CI check to make sure migration guide is updated if we have API changes.

This should give a warning instead of failing immediately. Some API changes add new methods (such as adding new APIs), while others are source compatible (such as adding a type parameter <T> which can be automatically inferred). Those changes don't need the binding to be changed.

Sounds good. I added the new check to the ignore list of merge-check.

Edit: I thought I pushed this 72b236e.

…nge-log' into feature/api-change-log

wks · 2024-05-13T08:53:46Z

I changed the style so that the first two levels of the lists form an outline of the changes, and the third level of the list contains more details and explanations.

I also use JavaScript to let the reader collapse and expand the third level of the lists. When collapsed, the list will look like a terse outline, but the readers can still expand individual items if they desire. This functionality is only available in the rendered HTML version.

I also moved the template from the comments into a dedicated page.

qinsoon · 2024-05-14T01:03:20Z

This is what the current form looks like for one API breaking PR, when we hide 'collapsible' sections.

Users need to be able to locate the information they need by looking at the list. If we have 10 entries like above in the list, the users won't be able to easily find the entry that they are looking for. You can just try this yourself: copy and paste that one entry 10 times, and see if you can easily locate the title for each entry. I find it hard. If users cannot even find the titles easily, they will have a hard time to find the right entry for their needs.

I suggest something like this.

### `ObjectReference` is no longer nullable

**TL;DR** `ObjectReference` can no longer represent a NULL reference.  Some methods of
`ObjectReferences` and write barrier functions are changed.  VM bindings need to re-implement
methods of the `Edge`, `ObjectModel` and `ReferenceGlue` traits.

COLLAPSIBLE START
All the detailed description goes here
COLLAPSIBLE END

We can use something like https://crates.io/crates/mdbook-admonish for collapsible blocks.

wks · 2024-05-14T01:53:13Z

Users need to be able to locate the information they need by looking at the list.

In my opinion, the information the VM binding developers need the most is the detailed guide to change a particular type/method when they don't know how to fix that compilation error. So there needs to be an option to expand everything (or just the type and method names) and let the developer search for the method name they want to locate.

But it is nice to have an option to hide everything other than the TL;DR part just in case someone wants to skim through all changes and see if it is worth upgrading to a particular version. I'll make the change.

wks · 2024-05-14T01:59:25Z

We can use something like https://crates.io/crates/mdbook-admonish for collapsible blocks.

Thank you. That's something I was looking for. I even tried to replicate what mdbook-admonish does. But one problem with that is that Admonish is intended to draw attention. But I want the folded details to be as quiet as possible so that the reader can focus on things that are not folded. I think we can use Admonish for the TL;DR part, instead.

qinsoon · 2024-05-14T02:15:19Z

So there needs to be an option to expand everything (or just the type and method names) and let the developer search for the method name they want to locate.

In your implementation, you use the <details> tag so those collapsible sections are rendered in DOM. You can do in-page search from browsers to find text in collapsed sections without the need to expanding them first. I think that is nice.

qinsoon · 2024-05-14T02:17:55Z

I think we can use Admonish for the TL;DR part, instead.

If you mean use admonish to highlight TLDR, that's fine. But don't make TLDR collapsible.

wks · 2024-05-14T07:18:21Z

Now we have the option to fold everything except the TL;DR.

qinsoon

Format-wise, I think it is good now. I have some questions about the content.

Also there is one question we need to answer: do we expect external contributors to write the migration guide when they introduce a API breaking change? We could make it optional, and require maintainers help write the migration guide instead.

docs/userguide/src/migration/prefix.md

docs/userguide/src/migration/template.md

qinsoon · 2024-05-15T00:59:13Z

docs/userguide/src/migration/prefix.md

+        +   Note: The `Edge` trait does not have a method for storing NULL to a slot.  The VM
+            binding needs to implement its own method to store NULL to a slot.
+
+Miscellaneous changes:


Why do we need a separate title here? The things here seem to belong to the 'API changes' part.

Because strictly speaking, the API is not changed w.r.t. get_finalized_object and get_forwarded_object. They returned Option<ObjectReference> before, and they still return Option<ObjectReference>. However, it is worth mentioning because the binding may expose them to C/C++, and that will fail to compile if the extern "C" functions return ObjectReference which was previously nullable.

I see. You mean the API is compliant but the semantic is changed. Are there any changes other than semantic changes that should be put as 'miscellaneous changes'?

The semantics is not changed, either. get_finalized_object used to return Option<ObjectReference> which meant "there is no finalized object". It still returns Option<ObjectReference> and still means the same. The problem is how the VM binding exposes it to C++. MMTk does not provide an official C/C++ API, so the exposed extern "C" wrapper is not controlled by MMTk-core. I mentioned that because many bindings do that, i.e. they use ObjectReference::NULL to encode None, which is no longer possible.

Strictly speaking, the root cause of that is that ObjectReference::NULL became non-nullable. The extern "C" wrappers of get_finalized_object and get_forwarded_object are just two possible use cases of ObjectReference::NULL. But since we also introduced a NullableObjectReference, we mention that in "miscellaneous changes".

If there are other things that have non-obvious but better way to do, we can mention them in "miscellaneous changes", too. And of course there are other use cases that I can't foresee now.

I changed it to "Not API change, but worth noting". The purpose should be clear now.

qinsoon · 2024-05-15T01:04:12Z

docs/userguide/src/SUMMARY.md

@@ -36,6 +36,8 @@
    - [Performance Tuning](portingguide/perf_tuning/prefix.md)
        - [Link Time Optimization](portingguide/perf_tuning/lto.md)
        - [Optimizing Allocation](portingguide/perf_tuning/alloc.md)
+- [API Migration Guide](migration/prefix.md)
+    - [Template (for mmtk-core developers)](migration/template.md)


Do we even need to list the template in mdbook? For people who need to edit the migration guide, they will view the template as the raw markdown file rather than the rendered HTML version. For people who do not edit the migration guide, they do not need to see this template in the migration guide at all.

I prefer them rendered so that editors know what to expect when looking at the rendered version. But I agree that ordinary readers may not need to read it. I'll see if we can make it a hidden section.

Now I use mdbook-hide to hide the chapter, but only in the published version. It is shown if we build the mdbook locally.

docs/userguide/assets/snippets/view-controls.html

docs/userguide/book.toml

wks · 2024-05-22T06:12:26Z

I simply removed the sub-title "VM bindings should reimplement ..." and merged the items into "API changes".

qinsoon · 2024-05-22T06:23:07Z

Broken link found:

### Errors in docs/userguide/src/portingguide/howto/nogc.md

* [404] [https://docs.mmtk.io/api/mmtk/vm/edge_shape/trait.MemorySlice.html](https://docs.mmtk.io/api/mmtk/vm/edge_shape/trait.MemorySlice.html) | Failed: Network error: Not Found

It should be from the last PR that renamed slot.

qinsoon · 2024-05-22T06:24:20Z

I simply removed the sub-title "VM bindings should reimplement ..." and merged the items into "API changes".

The template is not updated.

wks · 2024-05-22T06:43:24Z

I have just updated the template.

qinsoon

LGTM

API migration guide.

c303328

An *API Migration Guide* section is added to the porting guide. A symbolic link is created as `mmtk-core/API-MIGRATION.md` which can be easily discovered from the root of the repository.

wks requested a review from qinsoon May 6, 2024 09:43

qinsoon added 2 commits May 6, 2024 23:51

Check if the migration guide is updated when we have API change

0fa5d80

Move migration guide to a separate folder

7e88e32

qinsoon reviewed May 7, 2024

View reviewed changes

qinsoon added 3 commits May 7, 2024 01:13

Test API change

10b29e9

Fix the condition for running migration doc check

06eb989

Revert API change.

65ffd15

wks and others added 4 commits May 7, 2024 13:15

Changed to the bullet point style

0db7a48

Ignore migration guide check in merge check

72b236e

Not that concise. Index by type or module

fcd3fc2

Add template and adjust style

a8791d5

wks mentioned this pull request May 13, 2024

Rename edge to slot #1134

Merged

wks added 4 commits May 13, 2024 11:05

Merge remote-tracking branch 'refs/remotes/wks-github/feature/api-cha…

be69dd5

…nge-log' into feature/api-change-log

Remove irrelevant comments and do minor changes

5a3ce27

Collapsible details and isolated template.

0e69c51

Merge branch 'master' into feature/api-change-log

b617b28

Do not run the JS code outside API migration guide

4aefd6e

Formatting guide for Vim and VSCode.

39b4e42

wks added 3 commits May 14, 2024 14:37

Use mdbook-admonish for tl;dr

09e0fcd

Allow showing TL;DR only

72c9ae2

Minor changes to tl;dr

9fc6328

wks added 2 commits May 14, 2024 14:53

Move files around

5d049c8

Do not fold the next H2 into the details tag

b9f4fb7

qinsoon reviewed May 15, 2024

View reviewed changes

wks added 3 commits May 15, 2024 12:56

Clarify about qualified names and "only affect ..."

ed1a383

Allow hidding the template when publishing.

63047cf

Remove redundant tag in book.toml

ca78aab

qinsoon reviewed May 15, 2024

View reviewed changes

docs/userguide/book.toml Outdated Show resolved Hide resolved

qinsoon and others added 6 commits May 15, 2024 19:06

Update docs/userguide/book.toml

247229d

Merge branch 'master' into feature/api-change-log

0dbac85

Add changes for edge -> slot

26cdf9f

Remove the "VM bindings should reimplement..." title

f3d2902

Add links for the edge -> slot change

8f93d0f

Minor changes

bfa1de6

Update template

1ccc050

qinsoon approved these changes May 22, 2024

View reviewed changes

wks added this pull request to the merge queue May 22, 2024

Merged via the queue into mmtk:master with commit 170630f May 22, 2024
23 of 24 checks passed

wks deleted the feature/api-change-log branch May 22, 2024 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API migration guide. #1133

API migration guide. #1133

wks commented May 6, 2024

qinsoon left a comment

qinsoon May 7, 2024

wks May 7, 2024

qinsoon May 7, 2024

qinsoon May 7, 2024

wks May 7, 2024

qinsoon May 7, 2024

wks May 8, 2024 •

edited

wks commented May 7, 2024

qinsoon commented May 7, 2024 •

edited

wks commented May 13, 2024 •

edited

qinsoon commented May 14, 2024

wks commented May 14, 2024

wks commented May 14, 2024

qinsoon commented May 14, 2024

qinsoon commented May 14, 2024

wks commented May 14, 2024

qinsoon left a comment

qinsoon May 15, 2024

wks May 15, 2024

qinsoon May 15, 2024

wks May 15, 2024

wks May 22, 2024

qinsoon May 15, 2024

wks May 15, 2024

wks May 15, 2024

wks commented May 22, 2024

qinsoon commented May 22, 2024 •

edited

qinsoon commented May 22, 2024

wks commented May 22, 2024

qinsoon left a comment

API migration guide. #1133

API migration guide. #1133

Conversation

wks commented May 6, 2024

qinsoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wks May 8, 2024 • edited

Choose a reason for hiding this comment

wks commented May 7, 2024

qinsoon commented May 7, 2024 • edited

wks commented May 13, 2024 • edited

qinsoon commented May 14, 2024

wks commented May 14, 2024

wks commented May 14, 2024

qinsoon commented May 14, 2024

qinsoon commented May 14, 2024

wks commented May 14, 2024

qinsoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wks commented May 22, 2024

qinsoon commented May 22, 2024 • edited

qinsoon commented May 22, 2024

wks commented May 22, 2024

qinsoon left a comment

Choose a reason for hiding this comment

wks May 8, 2024 •

edited

qinsoon commented May 7, 2024 •

edited

wks commented May 13, 2024 •

edited

qinsoon commented May 22, 2024 •

edited