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.

Sign up for GitHub

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

Jump to bottom

remove Data from marshal and all packages that used it #2632

Merged
merged 2 commits into from
Mar 17, 2021
Merged

remove Data from marshal and all packages that used it #2632

merged 2 commits into from
Mar 17, 2021

Conversation

warner
Copy link
Member

@warner warner commented Mar 13, 2021

This completes the "empty objects should be pass-by-copy" transition plan.

refs #2018

@warner warner self-assigned this Mar 13, 2021
erights
erights approved these changes Mar 13, 2021
View reviewed changes
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM, including all the changes to marshal.

@warner
Copy link
Member Author

warner commented Mar 13, 2021

This currently fails CI in a Zoe unit test when run under XS. We think this is because of a weird XS bug that we're still trying to narrow down (maybe related to #2633). I won't land it while that bug is causing CI to fail, but if we change CI to not have XS failures flunk the overall build (until we get XS working with the full test suite), then I could land it while an XS-specific problem was still happening.

@warner warner mentioned this pull request Mar 14, 2021
Closed
@warner
Copy link
Member Author

warner commented Mar 15, 2021

The XS bug is going to be fun. There are 72 tests in test-zcf.js, and it is the 58th test that fails. If I delete the 60th test and beyond, the bug persists. If I then delete the 59th test (the one immedately following the failure), the bug goes away. The bug is somehow looking into the near future. Very much smells like a GC issue. Also, I can make the bug go away by allocating 800 or more empty objects in an earlier test (allocating 790 or fewer retains the failure).

@warner warner force-pushed the 2018-remove-Data branch 2 times, most recently from 28dc10b to a49ec7b Compare March 16, 2021 05:03
@warner
Copy link
Member Author

warner commented Mar 16, 2021 •
edited
Loading

Rats, the error persists even after upgrading to the newer XS code drop (#2655) that touched GC and WeakMaps somehow.

Because it provides a somewhat-stable trigger of this error, I'm going to hold off landing this while we continue to investigate.

@katelynsills
Copy link
Contributor

katelynsills commented Mar 16, 2021 •
edited
Loading

Rats, the error persists even after upgrading to the newer XS code drop (#2655) that touched GC and WeakMaps somehow.

Yup, it does. I was able to confirm this yesterday.

@warner warner force-pushed the 2018-remove-Data branch from a49ec7b to 037456a Compare March 17, 2021 17:59
warner added 2 commits March 17, 2021 11:00
@warner
fix: remove use of Data() from all packages
540d917 
Now that `harden({})` is pass-by-copy, we no longer need the Data() marker.

refs #2018
@warner
fix(marshal): remove Data
81dd9a4 
refs #2018
@warner warner force-pushed the 2018-remove-Data branch from 037456a to 81dd9a4 Compare March 17, 2021 18:01
@warner
Copy link
Member Author

warner commented Mar 17, 2021

Huh, well now it passes, both here and locally. I was sure I rebuilt everything the right way, so I don't know what changed. Joy.

I guess I'll land this now and keep my eyes open for any other XS failures.

@warner warner merged commit e7efb11 into master Mar 17, 2021
@warner warner deleted the 2018-remove-Data branch March 17, 2021 18:23
@warner warner mentioned this pull request Mar 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erights erights erights approved these changes

Assignees

@warner warner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@warner @katelynsills @erights