RavenDB-22261: Inconsistency in Tree external API #18422

redknightlois · 2024-04-18T19:04:28Z

Issue link

https://issues.hibernatingrhinos.com/issue/RavenDB-22261

Additional description

This is a series of commits aimed at homogenizing the external API of Voron data structures in order to simplify the code and avoid edge cases that arise from inconsistent usage patterns along the codebase.

Type of change

Bug fix
Regression bug fix
Optimization
New feature

How risky is the change?

Low
Moderate
High
Not relevant

Backward compatibility

Non breaking change
Ensured. Please explain how has it been implemented?
Breaking change
Not relevant

Is it platform specific issue?

Yes. Please list the affected platforms.
No

Documentation update

This change requires a documentation update. Please mark the issue on YouTrack using Documentation Required tag.
No documentation update is needed

Testing by Contributor

Tests have been added that prove the fix is effective or that the feature works
Internal classes added to the test class (e.g. entity or index definition classes) have the lowest possible access modifier (preferable private)
It has been verified by manual testing

Testing by RavenDB QA team

This change requires a special QA testing due to possible performance or resources usage implications (CPU, memory, IO). Please mark the issue on YouTrack using QA Required tag.
No special testing by RavenDB QA team is needed

Is there any existing behavior change of other features due to this change?

Yes. Please list the affected features/subsystems and provide appropriate explanation
No

UI work

It requires further work in the Studio. Please mark the issue on YouTrack using Studio Required tag.
No UI work is needed

redknightlois · 2024-04-18T22:43:50Z

Look at the last commit, this is more of an exploration for standarization of the logic inside Voron. I have a few other places where this approach can simplify a lot the current code. For example, as of right now I identified 10s of places where for example we assume implicitly non-nullness when in fact creation methods can return null among other issues.

cc @ayende @arekpalinski

src/Voron/Data/Fixed/FixedSizeTree.cs

arekpalinski · 2024-04-22T12:53:56Z

src/Voron/Data/Fixed/FixedSizeTree.cs

            if (header == null)
-                return;
+                return false;


Do you think it's a valid scenario to not read it? Shouldn't we revert the already introduced changes to tree object? For example we have already changed the name - tree._treeName = ...;

Yes, in fact I thought exactly as you the first time. Maybe I should add more comments. It can happen that we try to repurpose the instance but the instance does not exist / or does not have any item in it. BUT, we should still have repurposed the instance for a new tree. This is one of the big inconsistencies I have to address but I am not addressing it yet.

Any additional thoughts on this after spending more time on it?

arekpalinski · 2024-04-22T12:55:52Z

src/Voron/Data/Fixed/FixedSizeTree.cs

+                    if (tree._tx.Flags == TransactionFlags.ReadWrite && (tree._parent.State.Header.Flags & TreeFlags.FixedSizeTrees) != TreeFlags.FixedSizeTrees)
+                    {
+                        ref var state = ref tree._parent.State.Modify();
+                        state.Flags |= TreeFlags.FixedSizeTrees;


I have serious doubts if this is the place to handle that. It's only about repurposing the instance. IMO it's tricky that we modify the state here. I'd prefer more explicit approach

Agree, but that is the only way we can be backward compatible with already messed trees.

But in other places we explicitly apply TreeFlags.FixedSizeTrees flag

Why not to apply TreeFlags.FixedSizeTrees explicitly?

src/Raven.Server/Documents/Indexes/MapReduce/MapReduceIndexBase.cs

src/Voron/Data/Fixed/FixedSizeTree.cs

src/Voron/Impl/Transaction.cs

src/Voron/VoronExceptions.cs

src/Voron/Data/BTrees/Tree.cs

src/Voron/VoronExceptions.cs

maciejaszyk · 2024-05-07T08:41:18Z

src/Sparrow.Server/ByteString.cs

-            if (offset + count > dest.Length)
-                throw new ArgumentOutOfRangeException(nameof(from), "Cannot copy data after the end of the buffer");
-
+            ThrowIfOnDebug<InvalidOperationException>(HasValue == false, $"{nameof(HasValue)} is false.");
            EnsureIsNotBadPointer();


Is EnsureIsNotBadPointer left as a placeholder for future debugging? Since the method is empty.

It's a conditional implementation. This is the VALIDATE version of it:

[Conditional("VALIDATE")] internal readonly void EnsureIsNotBadPointer() { if (_pointer->Ptr == null) throw new InvalidOperationException("The inner storage pointer is not initialized. This is a defect on the implementation of the ByteStringContext class"); if (_pointer->Key == ByteStringStorage.NullKey) throw new InvalidOperationException("The memory referenced has already being released. This is a dangling pointer. Check your .Release() statements and aliases in the calling code."); if ( this.Key != _pointer->Key) { if (this.Key >> 16 != _pointer->Key >> 16) throw new InvalidOperationException("The owner context for the ByteString and the unmanaged storage are different. Make sure you havent killed the allocator and kept a reference to the ByteString outside of its scope."); Debug.Assert((this.Key & 0x0000000FFFFFFFF) != (_pointer->Key & 0x0000000FFFFFFFF), "(this.Key & 0x0000000FFFFFFFF) != (_pointer->Key & 0x0000000FFFFFFFF)"); throw new InvalidOperationException("The key for the ByteString and the unmanaged storage are different. This is a dangling pointer. Check your .Release() statements and aliases in the calling code."); } }

maciejaszyk · 2024-05-07T08:47:42Z

src/Sparrow.Server/ByteString.cs

@@ -1424,14 +1391,16 @@ private SegmentInformation AllocateSegment(int size)
        private void ThrowInvalidMemorySegmentOnAllocation()
        {
            AllocationFailed?.Invoke();
-            throw new InvalidOperationException("Allocate gave us a segment that was already disposed.");
+            PortableExceptions.Throw<InvalidOperationException>("Allocate gave us a segment that was already disposed.");


Suggested change

PortableExceptions.Throw<InvalidOperationException>("Allocate gave us a segment that was already disposed.");

Throw<InvalidOperationException>("Allocate gave us a segment that was already disposed.");

maciejaszyk · 2024-05-07T08:48:43Z

src/Sparrow.Server/ByteString.cs

-            if (_disposed)
-                ThrowObjectDisposed();
-
+            PortableExceptions.ThrowIfNull(value._pointer);


Suggested change

PortableExceptions.ThrowIfNull(value._pointer);

ThrowIfNull(value._pointer);

maciejaszyk · 2024-05-07T09:58:33Z

src/Sparrow/Json/UnmanagedWriteBuffer.cs

+            ThrowIf<InvalidOperationException>(required > ArenaMemoryAllocator.MaxArenaSize, 
+                $"Tried to allocate {new Size(required, SizeUnit.Bytes)}, which exceeds maximum allocation size of {new Size(ArenaMemoryAllocator.MaxArenaSize, SizeUnit.Bytes)}");


Don't we allocate to generate string exception every time even if we do not throw?

That's a good point. I will have to look into what the JIT does on that one. Most likely case is that it will get cancelled because the underlying Throw of the method is marked as [DoesNotReturn], but we should look into it anyways.

maciejaszyk · 2024-05-07T10:03:19Z

src/Voron/Data/BTrees/TreeIterator.cs

@@ -36,19 +42,18 @@ public TreeIterator(Tree tree, LowLevelTransaction tx, bool prefetch)

        public int GetCurrentDataSize()
        {
-            if (_disposed)
-                throw new ObjectDisposedException("TreeIterator " + _tree.Name);
+            ThrowIfDisposedOnDebug(this, "TreeIterator " + _tree.Name);


Do we want to throw only in debug?

It wasn't the case in the previous code. @redknightlois?

maciejaszyk · 2024-05-07T10:03:59Z

src/Voron/Data/BTrees/TreeIterator.cs

-
-                if (_currentPage == null)
-                    throw new InvalidOperationException("No current page was set");
+                ThrowIfDisposedOnDebug(this, "TreeIterator " + _tree.Name);


@redknightlois ?

maciejaszyk · 2024-05-07T10:11:06Z

test/FastTests/Voron/Trees/TreeRenaming.cs

@@ -8,6 +8,7 @@
 using Xunit;
 using Voron.Global;
 using Xunit.Abstractions;
+using Elastic.Clients.Elasticsearch;


Unintended?

I hate resharper and it's automatic additions. Yes.

it's still here

redknightlois · 2024-05-11T00:59:20Z

Test this in DEBUG

arekpalinski · 2024-05-13T05:59:46Z

test this in debug please

arekpalinski · 2024-05-13T09:42:19Z

test results in Debug - https://jenkins.hibernatingrhinos.com/job/PR_Tests_Debug/119/

arekpalinski · 2024-05-21T07:54:12Z

test this in debug please

arekpalinski · 2024-05-21T07:54:58Z

test/FastTests/Voron/Trees/TreeRenaming.cs

@@ -8,6 +8,7 @@
 using Xunit;
 using Voron.Global;
 using Xunit.Abstractions;
+using Elastic.Clients.Elasticsearch;


it's still here

arekpalinski · 2024-05-21T07:57:46Z

src/Corax/Querying/IndexSearcher.cs

@@ -212,7 +212,9 @@ internal Slice EncodeAndApplyAnalyzer(in FieldMetadata binding, Analyzer analyze
    //Function used to generate Slice from query parameters.
    //We cannot dispose them before the whole query is executed because they are an integral part of IQueryMatch.
    //We know that the Slices are automatically disposed when the transaction is closed so we don't need to track them.
+#if !DEBUG


Why only if not debug? I'm worried it might be a potential source of odd issues

The reason why you want it in debug is because if you dont do so, you can't debug this method as Visual Studio will trip into not initialized memory and kill the process. (It sucks big time).

Ouch :/ Okay I understand, although I'm still a bit concerned about having that only in RElease

arekpalinski · 2024-05-21T08:46:23Z

src/Sparrow/PortableExceptions.cs

+        [InterpolatedStringHandler]
+        public readonly ref struct ConditionalThrowInterpolatedStringHandler
+        {
+            private readonly StringBuilder? _builder;


Can we use #nullable enable in this file? So we won't need to enable it globally but use a file-by-file approach

OK, but at least for Voron, Sparrow and Corax expect to see in the next few month added usage of those annotations.

arekpalinski · 2024-05-21T08:46:59Z

src/Voron/Data/BTrees/TreeIterator.cs

@@ -36,19 +42,18 @@ public TreeIterator(Tree tree, LowLevelTransaction tx, bool prefetch)

        public int GetCurrentDataSize()
        {
-            if (_disposed)
-                throw new ObjectDisposedException("TreeIterator " + _tree.Name);
+            ThrowIfDisposedOnDebug(this, "TreeIterator " + _tree.Name);


It wasn't the case in the previous code. @redknightlois?

arekpalinski · 2024-05-21T08:47:40Z

src/Voron/Data/BTrees/TreeIterator.cs

-
-                if (_currentPage == null)
-                    throw new InvalidOperationException("No current page was set");
+                ThrowIfDisposedOnDebug(this, "TreeIterator " + _tree.Name);


@redknightlois ?

arekpalinski · 2024-05-21T08:49:35Z

src/Voron/Data/Fixed/FixedSizeTree.cs

            if (header == null)
-                return;
+                return false;


Any additional thoughts on this after spending more time on it?

arekpalinski · 2024-05-21T08:50:46Z

src/Voron/Data/Fixed/FixedSizeTree.cs

+                    if (tree._tx.Flags == TransactionFlags.ReadWrite && (tree._parent.State.Header.Flags & TreeFlags.FixedSizeTrees) != TreeFlags.FixedSizeTrees)
+                    {
+                        ref var state = ref tree._parent.State.Modify();
+                        state.Flags |= TreeFlags.FixedSizeTrees;


Why not to apply TreeFlags.FixedSizeTrees explicitly?

redknightlois · 2024-05-21T12:57:25Z

I don't understand why it does not allow me to comment on your comments. So I will go one by one with quotes, but first thing first. I hate resharper and it adding back stuff I explicitly remove.

Why not to apply TreeFlags.FixedSizeTrees explicitly?

Because one of the important changes is to make the ability to modify the state explicit. In fact, this uncovered a bug that had no impact but it was already there. It was an inconsistency on what we stored at the level of the parent.

Any additional thoughts on this after spending more time on it?

Yes, we are handling it explicitly on the caller now.

It wasn't the case in the previous code. @redknightlois?

I inspected every single call manually, there is no case where it makes sense to do that check on runtime. I still leave the check on Debug for 2 reasons.

It is an invariant check which I am fine with it being checked.
I will be moving more checks that meet the same criteria while we start modifying this files. Already did that for JsonOperationContext not long ago.

arekpalinski · 2024-05-22T10:20:18Z

test this in debug please

arekpalinski · 2024-05-23T06:14:01Z

test/FastTests/Voron/Trees/TreeRenaming.cs

@@ -53,7 +51,7 @@ public void CanRenameTree()
            }
        }

-        [Fact]
+        [RavenFact(RavenTestCategory.Voron)]


Since you switched to RavenFact please adjust - AllTestsShouldUseRavenFactOrRavenTheoryAttributes test

arekpalinski · 2024-05-23T06:18:30Z

@redknightlois what is the status here?

Are you going to make any more changes / profiling etc?
What about:

This change requires a special QA testing due to possible performance or resources usage implications (CPU, memory, IO). Please mark the issue on YouTrack using QA Required tag.

?
I believe this needs to be verified before we merge this PR CC @garayx

redknightlois · 2024-05-23T14:36:15Z

We need to do a performance run to avoid regressions, correct.

…tic methods.

…tures.

…ely.

Applied readonly modifiers where applicable Fixed mispellings Code that is no longer used because of replacements.

…ng interpolation method

github-actions bot added the v6.0 label Apr 18, 2024

redknightlois added the DO NOT MERGE label Apr 18, 2024

redknightlois requested review from ayende and arekpalinski and removed request for ayende April 18, 2024 22:44

ayende approved these changes Apr 19, 2024

View reviewed changes

redknightlois force-pushed the RavenDB-22261 branch from 85cf414 to d214c6f Compare April 20, 2024 01:36

arekpalinski reviewed Apr 22, 2024

View reviewed changes

arekpalinski requested a review from maciejaszyk April 22, 2024 13:05

redknightlois force-pushed the RavenDB-22261 branch 3 times, most recently from b74b208 to 3a7fdf5 Compare April 26, 2024 16:14

redknightlois force-pushed the RavenDB-22261 branch 4 times, most recently from 8f6ada6 to 84f64f8 Compare May 3, 2024 20:14

redknightlois removed the DO NOT MERGE label May 3, 2024

arekpalinski self-requested a review May 6, 2024 07:34

redknightlois force-pushed the RavenDB-22261 branch from 84f64f8 to 9f1159e Compare May 6, 2024 16:18

maciejaszyk reviewed May 7, 2024

View reviewed changes

redknightlois force-pushed the RavenDB-22261 branch 2 times, most recently from 81b3aa2 to 17b5d13 Compare May 10, 2024 19:29

redknightlois force-pushed the RavenDB-22261 branch 3 times, most recently from 9ec7724 to d45f744 Compare May 15, 2024 18:19

arekpalinski reviewed May 21, 2024

View reviewed changes

redknightlois force-pushed the RavenDB-22261 branch from d45f744 to f7351cb Compare May 21, 2024 14:05

arekpalinski reviewed May 23, 2024

View reviewed changes

redknightlois added 16 commits May 27, 2024 10:27

RavenDB-22261: Refactoring in preparation for changing creation methods.

4e86239

RavenDB-22261: Accesses to trees are only able to be done through sta…

64389c4

…tic methods.

RavenDB-22261: Standarization of exceptions for read-only transactions

70a3a82

RavenDB-22261: DisposableQueryable and exceptions standarization.

aeee4d4

RavenDB-22261: Standarization of Sparrow low level primitives.

2327310

RavenDB-22261: Improvements to Encoder3Gram

5d4fb53

RavenDB-22261: More standarization and applied review comments

e73fe43

RavenDB-22261: LowLevelTransaction simplifying disposing conditions.

17e3609

RavenDB-22261: Be explicit when null is possible when accessing struc…

a7e31f3

…tures.

RavenDB-22261: Adjustments after profiling.

266ab2b

RavenDB-22261: ValueReader improved allocations

3a260c3

RavenDB-22261: Embedded Fixed Size Trees has to be handled appropriat…

b5d0dd6

…ely.

RavenDB-22261: Removed dead code and make some code more strict.

a9e0228

Applied readonly modifiers where applicable Fixed mispellings Code that is no longer used because of replacements.

RavenDB-22261: Implementation of exceptions using an alternative stri…

df4f0d1

…ng interpolation method

RavenDB-22261: Applying review comments.

4813d0f

RavenDB-22181: Avoid allocating cursor list.

43846a4

redknightlois force-pushed the RavenDB-22261 branch from f7351cb to 43846a4 Compare May 27, 2024 13:28

	PortableExceptions.Throw<InvalidOperationException>("Allocate gave us a segment that was already disposed.");
	Throw<InvalidOperationException>("Allocate gave us a segment that was already disposed.");

	PortableExceptions.ThrowIfNull(value._pointer);
	ThrowIfNull(value._pointer);

		ThrowIf<InvalidOperationException>(required > ArenaMemoryAllocator.MaxArenaSize,
		$"Tried to allocate {new Size(required, SizeUnit.Bytes)}, which exceeds maximum allocation size of {new Size(ArenaMemoryAllocator.MaxArenaSize, SizeUnit.Bytes)}");

RavenDB-22261: Inconsistency in Tree external API #18422

Are you sure you want to change the base?

RavenDB-22261: Inconsistency in Tree external API #18422

Conversation

redknightlois commented Apr 18, 2024 • edited

Issue link

Additional description

Type of change

How risky is the change?

Backward compatibility

Is it platform specific issue?

Documentation update

Testing by Contributor

Testing by RavenDB QA team

Is there any existing behavior change of other features due to this change?

UI work

redknightlois commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redknightlois May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redknightlois commented May 11, 2024

arekpalinski commented May 13, 2024

arekpalinski commented May 13, 2024

arekpalinski commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redknightlois commented May 21, 2024 • edited

arekpalinski commented May 22, 2024

Choose a reason for hiding this comment

arekpalinski commented May 23, 2024

redknightlois commented May 23, 2024

redknightlois commented Apr 18, 2024 •

edited

redknightlois May 7, 2024 •

edited

redknightlois commented May 21, 2024 •

edited