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

Backport new constant for Marshal.load(record.dump) forward compatibility #39769

Merged

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jul 1, 2020

The purpose of backporting #39612 is for Marshal.load(record.dump)
forward compatibility.

Actually we never said we would keep Mashal compatibility between two
versions of Rails, so we sometimes directly added and removed the
internal objects. Theoretically, removing objects will lose backward
compatibility and adding objects will lose forward compatibility.

Example:

I'm not sure how much we should be aware of Mashal.load compatibility.
Especially if we should keep perfectly forward compatibility, we almost
have no chance to improve object layout (includes ivar usage).

One possible idea to mitigate breaking forward compatibility is
backporting new constant to the latest stable branch.

PERF: 45% faster attributes for readonly usage
The purpose of backporting rails#39612 is for `Marshal.load(record.dump)`
forward compatibility.

Actually we never said we would keep Mashal compatibility between two
versions of Rails, so we sometimes directly added and removed the
internal objects. Theoretically, removing objects will lose backward
compatibility and adding objects will lose forward compatibility.

Example:

* Rails 4.2/4.1 lost forward/backward compatibility due to entirely changed object layout
* Rails 5.0/4.2 lost backward compatibility due to removed `MysqlDateTime`
* Rails 5.2/5.1 lost forward compatibility due to added `SQLite3Integer`
* Rails 5.2/5.1 lost forward/backward compatibility due to added `Type::Json` and removed `MysqlJson`/`OID::Json`

I'm not sure how much we should be aware of `Mashal.load` compatibility.
Especially if we should keep perfectly forward compatibility, we almost
have no chance to improve object layout (includes ivar usage).

One possible idea to mitigate breaking forward compatibility is
backporting new constant to the latest stable branch.
@rails-bot rails-bot bot added the activemodel label Jul 1, 2020
@kamipo kamipo merged commit 7376d2e into rails:6-0-stable Aug 22, 2020
@kamipo kamipo deleted the backport_new_constant_for_forward_compatibility branch August 22, 2020 15:49
@rafaelfranca
Copy link
Member

I'm not sure we should backport changes to stable branches to keep backwards compatibly with Marshal. Why did you decided to do this? I don't remember us trying to keep Marshal compatibility, but we definitely need to know when YAML dump changes.

@rafaelfranca
Copy link
Member

I remember now. I commented on #39612. I think it is fine to break Marshal.load compatibility, between minor versions of Rails. For us that we are always running on Rails master we can't invalidate caches based on the Rails version since it is always the same. I think the right solution for my problem is to use the git sha as the Rails version and not Rails backport commits to stable releases just to keep them compatible. I think you can revert this.

@kamipo
Copy link
Member Author

kamipo commented Aug 26, 2020

Sure, I'll revert this.

kamipo added a commit that referenced this pull request Aug 26, 2020
…or_forward_compatibility"

This reverts commit 7376d2e, reversing
changes made to 6dd78c5.
@kamipo
Copy link
Member Author

kamipo commented Aug 26, 2020

Reverted 60776ef.

@rafaelfranca
Copy link
Member

Thank @kamipo. And sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants