fix pitfall for comdat folding #15467
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
we have two static methods with identical code, which returned the addresses of the distinct method-local static objects. when enabling comdat folding, the compiler decided to merge the two functions into one, which gave both objects the same in-memory address. this lead to follow-up errors because the object addresses were used to tell two types of object (collections and views) apart. this was not only error-prone, but also complicated. this PR replaces all this with a simple enum class.
…-comdat-folding-pitfall
gnusi
suggested changes
Jan 6, 2022
| @@ -105,7 +105,7 @@ class LogicalCollection : public LogicalDataSource { | |||
| }; | |||
|
|
|||
| /// @brief the category representing a logical collection | |||
| static Category const& category() noexcept; | |||
There was a problem hiding this comment.
it has to remain static as member is already present in LogicalDataSource
There was a problem hiding this comment.
I don't think so. It is declared as pure virtual in LogicalDataSource.
arangod/VocBase/LogicalDataSource.h
Outdated
| @@ -124,7 +104,7 @@ class LogicalDataSource { | |||
|
|
|||
| virtual ~LogicalDataSource() = default; | |||
|
|
|||
| Category const& category() const noexcept { return _category; } | |||
| virtual Category category() const noexcept = 0; | |||
There was a problem hiding this comment.
let's just store Category as a member to avoid unnecessary virtual calls
There was a problem hiding this comment.
That would also work, but I dislike that approach for the reason that then in theory it would be possible to create a LogicalView with category kCollection. I prefer the category to be part of the derived classes, so it's error-proof to use (and one less data member to carry around and think about). Of course this approach has the virtual calls as a downside, but I wouldn't expect any actual performance regression in our tests. Normally the category is determined once or maybe twice per LogicalDataSource for a high-level operation. E.g. an AQL query iterating over the docs in a collection will determine the category once or twice at start, but the actual query execution will not do it.
So it shouldn't really matter IMO.
There was a problem hiding this comment.
@jsteemann we can add an assertion in LogicalCollection/LogicalView to ensure that category is correct
gnusi
approved these changes
Jan 6, 2022
mpoeter
approved these changes
Jan 7, 2022
markuspf
pushed a commit
that referenced
this pull request
Jan 10, 2022
Co-authored-by: Andrey Abramov <andrey@arangodb.com>
maierlars
pushed a commit
that referenced
this pull request
Jan 28, 2022
* Setup leader state machine functions * Datastructure and test setup * Skeletonizign * First test code passes * Implement Leadership election * Implement some tests for running a leaderelection campaign * velocyPackHell * add forgotten file * Split work up * ... * Fix compiler complaints * Address linter complaints * temp: update clang-format.yml attempting to fix bad git diff * Revert "temp: update clang-format.yml" This reverts commit 02cda31. * some light refactoring * Fix bug in runElectionCampaign * Add a test for leadership election terms * Fix test comments and name * Handle a corner case differently * Remove rendundant comment * Move to unified representation of Agency * Rename files to make clearer what is supposed to to what * Remove some no longer used files * Add code for checking out target * Add some stub code * add more indexHints: `disableIndex` and `maxProjections` (#15446) Co-authored-by: Michael Hackstein <michael@arangodb.com> * fix pitfall for comdat folding (#15467) Co-authored-by: Andrey Abramov <andrey@arangodb.com> * small code cleanup (#15474) * make it 2022 (#15466) * [CINFRA-161] Follower State Transfer (#15352) * Added log core to leader and follower. * Rough sketch of snapshot transfer on followers. * Added ILogLeader and ILogFollower interfaces. * Optimized LogMultiplexer. Less copies of shared_ptrs. * Added FakeReplicatedState. * Fixing tests. * Preparations for FollowerState tests. * Renamed FakeFollower to FakeAbstractFollower. * Added new FakeFollower. * Moved interfaces to a separate header file. * Moved LeaderStateManager and FollowerStateManager into sep. files. * Completed first test run. * Fixing use of temporary after expression. * Code cleanup. * Added missing include for mac. * One more try for apple clang. * Dropping std::totally_ordered because it is not supported by Jenkins apple-clang, using typename instead. * Minor formatting * Applied clang-format * Reordered sources * Fixed merge conflict * Reformatted *.tpp files * Switched to ParticipantResignedException * Update arangod/Replication2/ReplicatedState/StateInterfaces.h Co-authored-by: Alex Petenchea <alexandru.petenchea@arangodb.com> * Use less special pointer. Co-authored-by: apetenchea <alexandru.petenchea@arangodb.com> Co-authored-by: Tobias Gödderz <tobias@arangodb.com> * Move ReplicatedLog code into the correct place * Make it build * Fix compilation * Use new code for supervision. * add test * Tests pass (or are deleted) * Added test for setting a new leader. * Sketch out checkReplicatedLog * Stub out actions * Bulk * some more tests for Supervision functions * Add some more harness code * Remove LOG_DEVEL * Added supervision trace if configured. * all of it * Fix supervision path * Update participant flags * Dictate leadership * Added more tests for supervision. * Resume all servers after each test. * Added expected actions for changeLeader. * More tests. * Added experimental tests. * LeaderElection * Refactoring and cleanup of leader election * Fix simple replication2 failover test * Cleanup * Remove stray console.log calls * Fix testChangeLeader * make more tests pass * More tests 🎉 * Implement UpdateConfigAction * Implement EvictLeader * Added yet another test. * An excluded server is not electible * Remove redundant files * Remove redundant files fixes * Fixing unittests * Fix embarassing crash * Fix embarassing crash * Enable Leader Replacement * Fixing testCheckSimpleFailover. Sometimes leader was server[1]. * Added descriptions to tests. Exception handling supervision. * Address comments * remove stray file * make clang-12 compile me again * Fix a bug in testReplaceLeaderWithNewFollower * Post errors to agency * Add Supervision Errors * removed targetConfig - discussed during review. * make tests compile again * Fix changing configuration through target * be careful about leadership selection in Target * Fix some tests * Fix CMakeLists.txt * make linter happy * make linter happy * Jslint. * Fix shell tests * Add a hack that fixes up when the user tries to add a log without participants * clang-format * Compiler nanny-ing * More nanny * clang format * clang format * MSVC complains that the defaulted constructor isn't defined So we move it into the cpp file. Great good. So C++. * Fix up some Agency Path cleanup fallout * re-enable test * Fix testLogStatus test Co-authored-by: aMahanna <anthony.mahanna@gmail.com> Co-authored-by: Jan <jsteemann@users.noreply.github.com> Co-authored-by: Michael Hackstein <michael@arangodb.com> Co-authored-by: Andrey Abramov <andrey@arangodb.com> Co-authored-by: Lars Maier <lars@arangodb.com> Co-authored-by: apetenchea <alexandru.petenchea@arangodb.com> Co-authored-by: Tobias Gödderz <tobias@arangodb.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Scope & Purpose
Enterprise companion PR: https://github.com/arangodb/enterprise/pull/848
we have had 2 static methods with identical code, which returned the
addresses of the distinct method-local static objects. when enabling
comdat folding, the compiler decided to merge the two functions into
one, which gave both objects the same in-memory address.
this lead to follow-up errors because the object addresses were used to
tell two types of object (collections and views) apart.
this was not only error-prone, but also complicated. this PR replaces
all this with a simple enum class.
Testing & Verification