Bug fix/cleanup register planning #11336

jsteemann · 2020-03-25T21:11:20Z

Scope & Purpose

Clean up the AQL register planning code a bit, and simplify APIs a bit.

Strictly new functionality (i.e. a new feature / new option, no need for porting)
The behavior change can be verified via automatic tests

Testing & Verification

This change is already covered by existing tests, such as scripts/unittest shell_server_aql, scripts/unittest catch.

http://172.16.10.101:8080/view/PR/job/arangodb-matrix-pr/9164/

…anup-register-planning

jsteemann · 2020-03-26T10:37:20Z

http://172.16.10.101:8080/view/PR/job/arangodb-matrix-pr/9167/

jsteemann · 2020-03-26T12:40:56Z

http://172.16.10.101:8080/view/PR/job/arangodb-matrix-pr/9168/
(blue)

mpoeter · 2020-03-26T14:51:12Z

Just out of curiosity - why do you sort the RegisterId vectors? I did not see anything that relies on the order (though I could have easily missed that).

jsteemann · 2020-03-26T14:57:32Z

You are right, nothing reies on the order of the registers in the vector.
However, I am sorting them so when we walk over them we will be accessing registers in ascending order. Often, when a register is accessed its underlying data is accessed as well. If we don't access registers in random order but in ascending order, we may benefit from cache prefetching, at least in theory. Though I doubt this will have a great effect in reality.

goedderz

Thanks for the cleanup in the RegisterPlan!

I have two notes, though:

I'd like to keep the set of input registers in the ExecutorInfos. While it's currently not in use, it is useful and I want to use it when working on the register planning in the next weeks.
The change from std::unordered_set<RegisterId> to a sorted std::vector<RegisterId> is too global for my taste. A local copy of the set in ExecutorInfos or so would have sufficed for the performance gain, while keeping the changes more local and not encumbering all future work with sorting a vector everywhere, which surely someone will not know or not remember when making a change. Alternatively some kind of minimal wrapper would help, so not everyone has to remember sorting it everytime (e.g. a class RegisterSet with only a constructor, emplace, begin and end).

goedderz · 2020-03-30T13:31:04Z

arangod/Aql/ExecutorInfos.cpp

+  // effects when peeking at the registers' values in the AqlItemBlock later).
+#ifdef ARANGODB_ENABLE_MAINTAINER_MODE
+  TRI_ASSERT(std::is_sorted(_registersToKeep.begin(), _registersToKeep.end()));
+  TRI_ASSERT(std::is_sorted(_registersToClear.begin(), _registersToClear.end()));


Did you intentionally omit a similar check for _outRegs?

Suggested change

TRI_ASSERT(std::is_sorted(_registersToClear.begin(), _registersToClear.end()));

TRI_ASSERT(std::is_sorted(_registersToClear.begin(), _registersToClear.end()));

TRI_ASSERT(std::is_sorted(_outRegs.begin(), _outRegs.end()));

goedderz · 2020-03-30T13:34:37Z

arangod/Aql/ExecutorInfos.cpp

    // sic: It's possible that a current output register is immediately cleared!
    TRI_ASSERT(regToClear < nrOutputRegisters);
-    TRI_ASSERT(_registersToKeep->find(regToClear) == _registersToKeep->end());
+    TRI_ASSERT(std::find(_registersToKeep.begin(), _registersToKeep.end(), regToClear) == _registersToKeep.end());


Suggested change

TRI_ASSERT(std::find(_registersToKeep.begin(), _registersToKeep.end(), regToClear) == _registersToKeep.end());

TRI_ASSERT(!std::binary_search(_registersToKeep.begin(), _registersToKeep.end(), regToClear));

goedderz · 2020-03-30T13:35:04Z

arangod/Aql/ExecutorInfos.cpp

+  for (auto const& regToKeep : _registersToKeep) {
    TRI_ASSERT(regToKeep < nrInputRegisters);
-    TRI_ASSERT(_registersToClear->find(regToKeep) == _registersToClear->end());
+    TRI_ASSERT(std::find(_registersToClear.begin(), _registersToClear.end(), regToKeep) == _registersToClear.end());


Suggested change

TRI_ASSERT(std::find(_registersToClear.begin(), _registersToClear.end(), regToKeep) == _registersToClear.end());

TRI_ASSERT(!std::binary_search(_registersToClear.begin(), _registersToClear.end(), regToKeep));

goedderz · 2020-03-30T13:44:56Z

arangod/Aql/ExecutorInfos.h

-   *
-   * @return The indices of the input registers.
-   */
-  std::shared_ptr<std::unordered_set<RegisterId> const> getInputRegisters() const;


I would like to keep this information (the set of input registers). It will make it easy to add useful assertions to AQL that will immediately help when rewriting the register planning.

goedderz · 2020-04-02T15:08:46Z

arangod/Aql/InputAqlItemRow.cpp

+      // TODO: improve on this
+      if (std::find(registers.begin(), registers.end(), col) == registers.end()) {


Suggested change

// TODO: improve on this

if (std::find(registers.begin(), registers.end(), col) == registers.end()) {

if (!std::binary_search(registers.begin(), registers.end(), col)) {

goedderz · 2020-04-02T15:19:53Z

arangod/Aql/RegisterPlan.cpp

  totalNrRegs++;
 }

+void RegisterPlan::registerVariable(Variable const* v) {


goedderz · 2020-04-02T15:55:53Z

arangod/Aql/ExecutionNode.h


  /// @brief get registers to clear
-  std::unordered_set<RegisterId> const& getRegsToClear() const;
+  std::vector<RegisterId> const& getRegsToClear() const;


This is unnecessary and counterproductive. While I agree that there might be a performance gain when this is used in the OutputAqlItemRow, everywhere else it's just harder to read (that something is a vector gives me much less information than something being some kind of set) and error-prone. Anyone doing changes to any register set now has to know and remember that it has to be sorted.

jsteemann · 2020-04-03T23:58:15Z

Obsoleted by #11385

jsteemann added 6 commits March 25, 2020 10:37

initial work

dbb7430

Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/cle…

7d7103e

…anup-register-planning

remove unnecessary inputRegisters parameter from ExecutorInfos

90db99e

clean up some unnecessary code

78189d9

switch from std::unordered_set to std::vector

80552c6

Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/cle…

5de5db9

…anup-register-planning

jsteemann added this to the devel milestone Mar 25, 2020

jsteemann added 2 commits March 25, 2020 22:16

Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/cle…

381ed81

…anup-register-planning

more API cleanup

13009ae

jsteemann marked this pull request as ready for review March 26, 2020 10:37

jsteemann requested a review from goedderz March 26, 2020 10:37

jsteemann added 2 commits March 26, 2020 12:37

fix compile warnings

6615d7a

some micro optimizations

854bb9e

goedderz requested changes Apr 2, 2020

View reviewed changes

jsteemann mentioned this pull request Apr 3, 2020

simplify register planning code, a bit #11385

Merged

2 tasks

jsteemann closed this Apr 3, 2020

jsteemann deleted the bug-fix/cleanup-register-planning branch March 9, 2022 13:33

	TRI_ASSERT(std::is_sorted(_registersToClear.begin(), _registersToClear.end()));
	TRI_ASSERT(std::is_sorted(_registersToClear.begin(), _registersToClear.end()));
	TRI_ASSERT(std::is_sorted(_outRegs.begin(), _outRegs.end()));

	TRI_ASSERT(std::find(_registersToKeep.begin(), _registersToKeep.end(), regToClear) == _registersToKeep.end());
	TRI_ASSERT(!std::binary_search(_registersToKeep.begin(), _registersToKeep.end(), regToClear));

	TRI_ASSERT(std::find(_registersToClear.begin(), _registersToClear.end(), regToKeep) == _registersToClear.end());
	TRI_ASSERT(!std::binary_search(_registersToClear.begin(), _registersToClear.end(), regToKeep));

		// TODO: improve on this
		if (std::find(registers.begin(), registers.end(), col) == registers.end()) {

	// TODO: improve on this
	if (std::find(registers.begin(), registers.end(), col) == registers.end()) {
	if (!std::binary_search(registers.begin(), registers.end(), col)) {

Bug fix/cleanup register planning #11336

Bug fix/cleanup register planning #11336

Uh oh!

Conversation

jsteemann commented Mar 25, 2020

Scope & Purpose

Testing & Verification

Uh oh!

jsteemann commented Mar 26, 2020

Uh oh!

jsteemann commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpoeter commented Mar 26, 2020

Uh oh!

jsteemann commented Mar 26, 2020

Uh oh!

goedderz left a comment

Choose a reason for hiding this comment

Uh oh!

goedderz Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

goedderz Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

goedderz Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

goedderz Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

goedderz Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

goedderz Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

goedderz Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

jsteemann commented Apr 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jsteemann commented Mar 26, 2020 •

edited

Loading