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

vibe.data.json: switch back to union for possible Json values #2206

Merged

Conversation

FeepingCreature
Copy link
Contributor

Fix Issue 2205? The crashes haven't happened since.

@FeepingCreature FeepingCreature force-pushed the fix/replace-hack-with-union branch 2 times, most recently from 6f746f0 to 8f5d5f9 Compare August 23, 2018 06:13
@FeepingCreature
Copy link
Contributor Author

I don't get that error...

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Aug 23, 2018

skip the assert for json equality with m_object at compiletime, since it confuses the compiler now apparently

edit: I just want to note that the Travis CI failures are spurious.

@FeepingCreature
Copy link
Contributor Author

ping

@FeepingCreature
Copy link
Contributor Author

I want to emphasize again that this commit is the difference between "hard hang in json code" and "no problems".

Ping!

@FeepingCreature FeepingCreature force-pushed the fix/replace-hack-with-union branch 2 times, most recently from 2483175 to 0550713 Compare January 27, 2021 15:25
@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jan 27, 2021

resurrecting this pr as per #2525

edit: uh wtf is with those CI failures, I don't get those locally even with that exact version...
edit: okay, I get them with dub test :data, but the line number is complete nonsense.

@FeepingCreature
Copy link
Contributor Author

Ah, turns out having ubyte[intEnum] in a union fucks you over when it comes to CTFE. Switched to a {size_t, size_t, size_t} struct.

@FeepingCreature
Copy link
Contributor Author

uh.

ping??

Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

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

Okay, so there is one error left for the oldest DMD/x86. If there is no obvious workaround for that, you can try to replace DMD 2.083.x with 2.084.x or 2.085.x, since that still falls within the "latest 10 minor releases" of support. Otherwise LGTM!

BTW, did you get the chance to verify the zeroFields workaround in production?

Yeah, and sorry letting this hang in limbo for so long. It looks safe enough now to make the switch without introducing regressions.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Feb 5, 2021

updated version to 2.084.1. I didn't test this specific version in production; the version we're deploying is still the one with the void*[] hack. I can switch to this one though, if you want data on effectiveness.

(You'll forgive me for not deliberately leaking ram on a production system for comparison, I hope. :) )

@s-ludwig
Copy link
Member

s-ludwig commented Feb 5, 2021

updated version to 2.084.1. I didn't test this specific version in production; the version we're deploying is still the one with the void*[] hack. I can switch to this one though, if you want data on effectiveness.
(You'll forgive me for not deliberately leaking ram on a production system for comparison, I hope. :) )

Sure, just validating that the new code really doesn't introduce a leak is definitely enough here ;) I'll also just merge it now, but it would be good if you would get the chance to try it out under controlled circumstances before a new release is tagged. I can do the same with the DUB registry, which is the most loaded service that I currently run. The leaks were showing through there pretty quickly if I remember right.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Feb 8, 2021

Ah, my genuine apologies. I was unable to convince folks to switch from a version that we think works to one that is not fully proven over the short term. I'll try to merge the union-based version to product master and test it as part of a future release, but we will only be able to confirm it fixes the bug without leaking in the medium future, ie. weeks to months.

@s-ludwig
Copy link
Member

s-ludwig commented Feb 9, 2021

I'll test with the dub-registry after I have collected some before-statistics. From my side, if that succeeds, that will be good enough to risk a release.

@s-ludwig
Copy link
Member

Okay, it's running for a few hours now and so far no change in memory consumption. I'll keep monitoring it, but it really should have shown by now. So it looks like this is fine!

@FeepingCreature FeepingCreature deleted the fix/replace-hack-with-union branch February 10, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants