ravendb icon indicating copy to clipboard operation
ravendb copied to clipboard

RavenDB-21952: Unified the whole compare infrastructure under Sparrow

Open redknightlois opened this issue 2 years ago • 2 comments

Issue link

https://issues.hibernatingrhinos.com/issue/RavenDB-21952 https://issues.hibernatingrhinos.com/issue/RavenDB-22068

Additional description

Given that we had different implementation in server and client many of the operations happening in the base Sparrow based code like Blittables were not able to use the updated AVX2 compare primitives. The work of unifying the Memory.Compare routines allow base Sparrow primitives to also benefit from the improvements.

Type of change

  • [ ] Bug fix
  • [ ] Regression bug fix
  • [x] Optimization
  • [ ] New feature

How risky is the change?

  • [ ] Low
  • [x] Moderate
  • [ ] High
  • [ ] Not relevant

Backward compatibility

  • [x] 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.
  • [x] No

Documentation update

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

Testing by Contributor

  • [ ] Tests have been added that prove the fix is effective or that the feature works
  • [x] 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

  • [x] 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
  • [x] No

UI work

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

redknightlois avatar Jan 16 '24 17:01 redknightlois

At FastTests.Sparrow.AdvMemoryTests.ExhaustiveIsEqualForEveryCombination you missed the case when literal has 64 characters. Which is supported: https://github.com/redknightlois/ravendb/blob/bdaee73cad61d3df4f8e195e6c4b54dcd13c8d46/src/Sparrow/Memory.cs#L901-L902

When you ran it it will return wrong result: image

maciejaszyk avatar Feb 12 '24 11:02 maciejaszyk

That's a current standing bug, only thing I did there is a rename of the method. Go ahead an issue a PR to fix that alone I will rebase this on Wednesday

redknightlois avatar Feb 12 '24 14:02 redknightlois

@redknightlois @maciejaszyk what is the status here, regarding the issue that Maciej reported in his comment?

arekpalinski avatar Feb 26 '24 13:02 arekpalinski

@redknightlois @maciejaszyk what is the status here, regarding the issue that Maciej reported in his comment?

Fixed: https://github.com/ravendb/ravendb/pull/18001/commits/3179cfb6b5026580d336bb1037b014e0c56eed27

redknightlois avatar Feb 26 '24 13:02 redknightlois

Awesome. Thanks

arekpalinski avatar Feb 26 '24 13:02 arekpalinski

If you could do the rebase and fix SlowTests.Tests.TestsInheritanceTests.AllTestsShouldUseRavenFactOrRavenTheoryAttributes that would be nice so we could merge the PR

arekpalinski avatar Feb 26 '24 13:02 arekpalinski