mmtk / mmtk-core

Memory Management ToolKit
https://www.mmtk.io
Other
379 stars 69 forks source link

API migration guide. #1133

Closed wks closed 6 months ago

wks commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 https://github.com/mmtk/mmtk-core/pull/1133/commits/72b236e7f1f0fce5ee50b1773f2f234d9d922dfe.

wks commented 6 months ago

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 commented 6 months ago

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

api-migration

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

wks commented 6 months ago

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

qinsoon commented 6 months ago

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 commented 6 months ago

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

The template is not updated.

wks commented 6 months ago

I have just updated the template.