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

implement box avoidance #2639

Merged
merged 5 commits into from
May 15, 2022
Merged

implement box avoidance #2639

merged 5 commits into from
May 15, 2022

Conversation

mgravell
Copy link
Contributor

@mgravell mgravell commented Jan 14, 2022

fix #2638

  1. adds internal BoxedPrimitives helper that holds boxed instances for common primitive values and has helper APIs
  2. use BoxedPrimitives in preference to vanilla boxing in JValue and JsonTextReader
  3. avoid new JValue(object) in key paths

Disclosure: if someone uses Unsafe methods to mutate the inside of one of these boxes: bad things. But that is self-inflicted.

Src/Newtonsoft.Json/Utilities/BoxedPrimitives.cs Outdated Show resolved Hide resolved
Src/Newtonsoft.Json/Utilities/BoxedPrimitives.cs Outdated Show resolved Hide resolved
Src/Newtonsoft.Json/Utilities/BoxedPrimitives.cs Outdated Show resolved Hide resolved
Src/Newtonsoft.Json/Utilities/BoxedPrimitives.cs Outdated Show resolved Hide resolved
@mgravell
Copy link
Contributor Author

@JamesNK nits resolved

@JamesNK
Copy link
Owner

JamesNK commented Jan 17, 2022

Does this have any impact on comparisons when the value is still boxed? I wonder about people doing something dumb like:

if (JValue1.Value == JValue2.Value)
{
}

Before it would be false but now it is true (if values are -1 to 8 and share a box)

@mgravell
Copy link
Contributor Author

mgravell commented Jan 18, 2022

@JamesNK yes, technically the change is observable; however, the code as shown is already incorrect for most definitions of "correct"/"incorrect"; when dealing with object, what they almost certainly mean is something like:

if (Equals(JValue1.Value, JValue2.Value))
{
}

or

if (JValue1.Value.Equals(JValue2.Value))
{
}

The == usage is simply ... wrong. It will already behave very inconsistently. For example, if the values are strings, it would currently report true for two empty strings but only two empty strings. The following passes against the old or new code:

        [Test]
        public void EmptyStringsAreSame()
        {
            var obj = (JObject)JToken.Parse(@"{""x"": """", ""y"": """"}");
            var x = ((JValue)obj["x"]).Value;
            var y = ((JValue)obj["y"]).Value;
            Assert.AreSame(x, y);
            Assert.AreEqual(x, y);
            Assert.IsTrue(x == y);
        }

        [Test]
        public void EqualStringsAreNotSame()
        {
            var obj = (JObject)JToken.Parse(@"{""x"": ""a"", ""y"": ""a""}");
            var x = ((JValue)obj["x"]).Value;
            var y = ((JValue)obj["y"]).Value;
            Assert.AreNotSame(x, y);
            Assert.AreEqual(x, y);
            Assert.IsFalse(x == y);
        }

I'm not sure we should be too concerned about obviously-broken code.

@JamesNK JamesNK merged commit 13717cf into JamesNK:master May 15, 2022
@JamesNK
Copy link
Owner

JamesNK commented May 15, 2022

Thanks @mgravell

dmmusil added a commit to dmmusil/Newtonsoft.Json that referenced this pull request Jan 15, 2023
This reverts commit 13717cf.

# Conflicts:
#	Src/Newtonsoft.Json/Utilities/BoxedPrimitives.cs
SimonCropp added a commit to SimonCropp/Argon that referenced this pull request Feb 11, 2023
sousapedro pushed a commit to sousapedro/Newtonsoft.Json-for-Unity that referenced this pull request Feb 21, 2023
Co-authored-by: Marc Gravell <[email protected]>
Co-authored-by: James Newton-King <[email protected]>
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.

Memory usage suggestion : reduce unnecessary boxes
2 participants