Use connected lists for tape data structure #954

rohanjulka19 · 2024-06-20T16:00:26Z

Currently I have added a basic implementation of tape data structure with connected lists/slabs. It is more or less similar to current tape data strucutre I copied a lot of code just added blocks and only added the functionality which I feel is needed for reverse mode.

Right now NumericalDiff also uses tape so I defined two types one is "tape" which points to the new tape implementation and "old_tape" which points to current tape data strcuture being used.

Tests are running fine for the new implementation.

These are the benchmarks for old_tape and new_tape.

LastTest-new-tape.log
LastTest-tape.log

Fixes - #793

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 16. Check the log or trigger a new build to see more.

github-actions · 2024-06-20T16:08:48Z

include/clad/Differentiator/NewTape.h

+//
+
+#ifndef CLAD_NEWTAPE_H
+#define CLAD_NEWTAPE_H


warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change

#define CLAD_NEWTAPE_H

#ifndef CLAD_DIFFERENTIATOR_NEWTAPE_H

#define CLAD_DIFFERENTIATOR_NEWTAPE_H

include/clad/Differentiator/NewTape.h:87:

- #endif // CLAD_NEWTAPE_H + #endif // CLAD_DIFFERENTIATOR_NEWTAPE_H

include/clad/Differentiator/NewTape.h

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 12. Check the log or trigger a new build to see more.

github-actions · 2024-06-20T16:24:24Z

include/clad/Differentiator/NewTape.h

@@ -0,0 +1,84 @@
+#ifndef CLAD_NEWTAPE_H
+#define CLAD_NEWTAPE_H


warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change

#define CLAD_NEWTAPE_H

#ifndef CLAD_DIFFERENTIATOR_NEWTAPE_H

#define CLAD_DIFFERENTIATOR_NEWTAPE_H

include/clad/Differentiator/NewTape.h:83:

- #endif // CLAD_NEWTAPE_H + #endif // CLAD_DIFFERENTIATOR_NEWTAPE_H

include/clad/Differentiator/NewTape.h

github-actions · 2024-06-20T16:24:24Z

include/clad/Differentiator/NewTape.h

+  void emplace_back(ArgsT&&... args) {
+    if (!cur_block || _size >= _capacity) {
+      Block<T>* prev_block = cur_block;
+      cur_block = new Block<T>(_capacity);


warning: assigning newly created 'gsl::owner<>' to non-owner 'Block *' [cppcoreguidelines-owning-memory]

cur_block = new Block<T>(_capacity); ^

github-actions · 2024-06-20T16:24:25Z

include/clad/Differentiator/NewTape.h

+      _size = 0;
+    }
+    _size += 1;
+    ::new (const_cast<void*>(static_cast<const volatile void*>(block_end())))


warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

::new (const_cast<void*>(static_cast<const volatile void*>(block_end()))) ^

include/clad/Differentiator/NewTape.h

vgvassilev · 2024-07-01T18:37:45Z

@rohanjulka19, do you mind addressing the clang-tidy complaints where relevant?

rohanjulka19 · 2024-07-01T18:47:41Z

hi, yeah will address these clang-tidy issues, actually i got stuck on the loop condition differentiation issue (once again) #818. But now I am fixing that one, once I push that fix will address these then.

github-actions

clang-tidy made some suggestions

github-actions · 2024-07-03T13:01:41Z

include/clad/Differentiator/NewTape.h

@@ -0,0 +1,87 @@
+#ifndef CLAD_NEWTAPE_H
+#define CLAD_NEWTAPE_H


warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change

#define CLAD_NEWTAPE_H

#ifndef CLAD_DIFFERENTIATOR_NEWTAPE_H

#define CLAD_DIFFERENTIATOR_NEWTAPE_H

include/clad/Differentiator/NewTape.h:86:

- #endif // CLAD_NEWTAPE_H + #endif // CLAD_DIFFERENTIATOR_NEWTAPE_H

include/clad/Differentiator/NewTape.h

github-actions · 2024-07-03T13:01:42Z

include/clad/Differentiator/NewTape.h

+  void emplace_back(ArgsT&&... args) {
+    if (!m_cur_block || m_size >= _capacity) {
+      Block<T>* prev_block = m_cur_block;
+      m_cur_block = new Block<T>(_capacity);


warning: assigning newly created 'gsl::owner<>' to non-owner 'Block *' [cppcoreguidelines-owning-memory]

m_cur_block = new Block<T>(_capacity); ^

include/clad/Differentiator/NewTape.h

github-actions

clang-tidy made some suggestions

include/clad/Differentiator/NewTape.h

github-actions · 2024-07-03T17:27:41Z

include/clad/Differentiator/NewTape.h

+  ~Block() {
+    destroy(block_begin(), block_end());
+    ::operator delete(
+        const_cast<void*>(static_cast<const volatile void*>(data)));


warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

const_cast<void*>(static_cast<const volatile void*>(data))); ^

github-actions · 2024-07-03T17:27:41Z

include/clad/Differentiator/NewTape.h

+  void emplace_back(ArgsT&&... args) {
+    if (!m_cur_block || m_size >= m_capacity) {
+      Block<T>* prev_block = m_cur_block;
+      m_cur_block = new Block<T>(m_capacity);


warning: assigning newly created 'gsl::owner<>' to non-owner 'Block *' [cppcoreguidelines-owning-memory]

m_cur_block = new Block<T>(m_capacity); ^

github-actions · 2024-07-03T17:27:41Z

include/clad/Differentiator/NewTape.h

+      m_size = 0;
+    }
+    m_size += 1;
+    ::new (const_cast<void*>(static_cast<const volatile void*>(end())))


warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

::new (const_cast<void*>(static_cast<const volatile void*>(end()))) ^

rohanjulka19 · 2024-07-04T12:12:31Z

@vgvassilev I have fixed most of the suggestion, the above four I don't think needs to be addressed.

vgvassilev · 2024-07-04T14:18:44Z

@vgvassilev I have fixed most of the suggestion, the above four I don't think needs to be addressed.

The include protector suggestions seem good. Looks like the new tape has a problem with cuda.

codecov · 2024-07-04T15:23:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.37%. Comparing base (d82f7fd) to head (26fcee4).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #954   +/-   ##
=======================================
  Coverage   94.37%   94.37%           
=======================================
  Files          50       50           
  Lines        8366     8366           
=======================================
  Hits         7895     7895           
  Misses        471      471

rohanjulka19 · 2024-07-04T18:37:54Z

The include protector suggestions seem good. Looks like the new tape has a problem with cuda.

Have changed the include protector to align with the suggestion. The cuda test is passing but I am not sure if it will work when this runs on more than one thread in parallel

vgvassilev · 2024-07-06T15:28:34Z

I am a little puzzled since the new tape implementation is supposed to be faster than the existing one but it seems consistently slower: https://github.com/vgvassilev/clad/actions/runs/9798455926/job/27056951548?pr=954#step:31:596

Do you have any clue what might be going wrong?

rohanjulka19 · 2024-07-06T16:31:50Z

I thought it was due to the small block size leading to more memory allocation but increasing the block size also didn't worked so will investigate this further

vgvassilev · 2024-07-19T20:41:02Z

@rohanjulka19, do we have a clue what might be still wrong for the benchmarks? Maybe we should profile it with something like heaptrack, kcachegrind or similar...

rohanjulka19 · 2024-07-19T20:59:23Z

oh yeah I tried with timer but i couldn't figure out but I had a intution that because I am assigning memory twice once for the block and once for the array inside the block maybe that's whats causing the issue. So I am trying to just assign memory once by taking a fixed array of size 32 that should allocate all the memory once, but I am not very good with cpp so there is some issue with either memory allocation or something else. I will investigate the code this weekend, what do you think can this be the performance issue ? Should I use heaptrack to check if this is true ?

github-actions

clang-tidy made some suggestions

github-actions · 2024-07-24T09:34:27Z

include/clad/Differentiator/NewTape.h

+
+template <typename T> class Block {
+public:
+  T data[capacity];


warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

T data[capacity]; ^

github-actions · 2024-07-24T09:34:27Z

include/clad/Differentiator/NewTape.h

+  CUDA_HOST_DEVICE void emplace_back(ArgsT&&... args) {
+    if (!m_cur_block || m_size >= capacity) {
+      Block<NonConstT>* prev_block = m_cur_block;
+      m_cur_block = new Block<NonConstT>();


warning: assigning newly created 'gsl::owner<>' to non-owner 'Block *' (aka 'Block<typename std::remove_cv::type> *') [cppcoreguidelines-owning-memory]

m_cur_block = new Block<NonConstT>(); ^

rohanjulka19 · 2024-07-24T12:40:17Z

include/clad/Differentiator/NewTape.h

+
+  ~new_tape_impl() {
+    if (m_cur_block) {
+      //       destroy();


Hi @vgvassilev so when I comment this line the time reduces. This function calls the destructors for all the blocks. This should never be called as we are destructing the block when we pop the last element from it, but this is to be safe in case tape is terminated without popping all the values due to some error. Now even though I have put this function inside the if stmt still this function is affecting the performance even when this is not being called. I believe this is because of function inlining. Is there way to ensure this doesn't happen ?

Probably that's right. If you compile the code with clang you can add -Rpass-missed=.* to get more information about the missed optimization opportunities.

rohanjulka19 · 2024-07-24T12:40:39Z

Additionally on my local for BM_ReverseModeProductExecute I am getting this time

BM_ReverseModeProductExecute 0.151 ns 0.150 ns 1000000000

But the benchmark returns this result

BM_ReverseModeProductExecute 114 ns 114 ns 5956940

What must be the reason for this ? Also I am on M2 Mac (ARM) so like what tool should I use to actually understand what's the issue ?

vgvassilev · 2024-07-24T12:45:03Z

include/clad/Differentiator/NewTape.h

+    if (m_size == 0) {
+      Block<NonConstT>* temp = m_cur_block;
+      m_cur_block = m_cur_block->prev;
+      temp->~Block<NonConstT>();


Why we do not delete the m_cur_block here?

Here we are deleting the current block at line 107.

I added the destroy function in the tape destructor to ensure that incase tape terminates without popping all the values, it should then call the destructor for all the pending blocks.

~new_tape_impl() { if (m_cur_block) { // destroy();

In this line I don't want the compiler optimization and instead keep the code of destroy function separate because if it is added to the destructor it will slow down the destructor even though the destroy function won't likely be called. If we are sure that all the values will always be popped before the tape terminates then I can just remove this function from the tape destructor

vgvassilev · 2024-07-24T12:47:39Z

include/clad/Differentiator/NewTape.h

+
+  ~new_tape_impl() {
+    if (m_cur_block) {
+      //       destroy();


Probably that's right. If you compile the code with clang you can add -Rpass-missed=.* to get more information about the missed optimization opportunities.

vgvassilev · 2024-07-24T12:48:46Z

Additionally on my local for BM_ReverseModeProductExecute I am getting this time

BM_ReverseModeProductExecute 0.151 ns 0.150 ns 1000000000

But the benchmark returns this result

BM_ReverseModeProductExecute 114 ns 114 ns 5956940

What must be the reason for this ? Also I am on M2 Mac (ARM) so like what tool should I use to actually understand what's the issue ?

Do you mean that the bots give different benchmark results? If so, that's expected because they are different machines with different properties.

rohanjulka19 · 2024-07-24T12:52:15Z

Additionally on my local for BM_ReverseModeProductExecute I am getting this time
BM_ReverseModeProductExecute 0.151 ns 0.150 ns 1000000000
But the benchmark returns this result
BM_ReverseModeProductExecute 114 ns 114 ns 5956940
What must be the reason for this ? Also I am on M2 Mac (ARM) so like what tool should I use to actually understand what's the issue ?

Do you mean that the bots give different benchmark results? If so, that's expected because they are different machines with different properties.

Yeah but in my laptop the new tape is perfoming better than old tape and according to the benchmarks exectued by the pipeline the old tape is doing better than the new tape

vgvassilev · 2024-07-24T13:54:41Z

Additionally on my local for BM_ReverseModeProductExecute I am getting this time
BM_ReverseModeProductExecute 0.151 ns 0.150 ns 1000000000
But the benchmark returns this result
BM_ReverseModeProductExecute 114 ns 114 ns 5956940
What must be the reason for this ? Also I am on M2 Mac (ARM) so like what tool should I use to actually understand what's the issue ?

Do you mean that the bots give different benchmark results? If so, that's expected because they are different machines with different properties.

Yeah but in my laptop the new tape is perfoming better than old tape and according to the benchmarks exectued by the pipeline the old tape is doing better than the new tape

Do you run your benchmarks on release builds?

Could you log in the bots and run some profiling there? You can do that by following: https://clad.readthedocs.io/en/latest/user/DevelopersDocumentation.html#debugging-github-runners

github-actions

clang-tidy made some suggestions

github-actions · 2024-09-12T22:21:42Z

include/clad/Differentiator/NewTape.h

+  using iterator = pointer;
+
+  CUDA_HOST_DEVICE Block() {
+  }


warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]

Suggested change

}

CUDA_HOST_DEVICE Block() = default;

github-actions · 2024-09-12T22:21:42Z

include/clad/Differentiator/NewTape.h

+public:
+  new_tape_impl() = default;
+
+  ~new_tape_impl() { }


warning: use '= default' to define a trivial destructor [modernize-use-equals-default]

Suggested change

~new_tape_impl() { }

~new_tape_impl() = default;

github-actions · 2024-09-12T22:21:42Z

include/clad/Differentiator/NewTape.h

+    if (m_size == 0) {
+      Block<NonConstT>* temp = m_cur_block;
+      m_cur_block = m_cur_block->prev;
+      delete temp;


warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]

delete temp; ^

Additional context

include/clad/Differentiator/NewTape.h:101: variable declared here

Block<NonConstT>* temp = m_cur_block; ^

Instead of a single list use mutliple connected lists to store elements. This allows to dynamically increase the size of tape without the need of relocating elements.

rohanjulka19 force-pushed the tape-impl branch from 949518f to c988543 Compare June 20, 2024 16:01

github-actions bot reviewed Jun 20, 2024

View reviewed changes

rohanjulka19 force-pushed the tape-impl branch from c988543 to 8fd8064 Compare June 20, 2024 16:09

github-actions bot reviewed Jun 20, 2024

View reviewed changes

rohanjulka19 force-pushed the tape-impl branch from 8fd8064 to 6ea4316 Compare July 3, 2024 12:54

github-actions bot reviewed Jul 3, 2024

View reviewed changes

rohanjulka19 force-pushed the tape-impl branch from 6ea4316 to 8bddf2d Compare July 3, 2024 17:21

github-actions bot reviewed Jul 3, 2024

View reviewed changes

rohanjulka19 force-pushed the tape-impl branch from 8bddf2d to 7db647d Compare July 4, 2024 15:15

rohanjulka19 force-pushed the tape-impl branch 2 times, most recently from 452b55d to 38fdfb3 Compare July 4, 2024 18:08

rohanjulka19 force-pushed the tape-impl branch from 38fdfb3 to 8e1575b Compare July 13, 2024 15:53

rohanjulka19 force-pushed the tape-impl branch from 8e1575b to 3fe7769 Compare July 24, 2024 09:31

github-actions bot reviewed Jul 24, 2024

View reviewed changes

rohanjulka19 commented Jul 24, 2024

View reviewed changes

vgvassilev reviewed Jul 24, 2024

View reviewed changes

rohanjulka19 force-pushed the tape-impl branch from 3fe7769 to 1cb9c4b Compare September 12, 2024 22:18

github-actions bot reviewed Sep 12, 2024

View reviewed changes

rohanjulka19 force-pushed the tape-impl branch from 1cb9c4b to 2495073 Compare September 12, 2024 22:37

rohanjulka19 force-pushed the tape-impl branch from 2495073 to 0a96b90 Compare October 17, 2024 10:10

Use connected lists for tape data structure

26fcee4

Instead of a single list use mutliple connected lists to store elements. This allows to dynamically increase the size of tape without the need of relocating elements.

rohanjulka19 force-pushed the tape-impl branch from 0a96b90 to 26fcee4 Compare October 17, 2024 11:18

	#define CLAD_NEWTAPE_H
	#ifndef CLAD_DIFFERENTIATOR_NEWTAPE_H
	#define CLAD_DIFFERENTIATOR_NEWTAPE_H

		@@ -0,0 +1,84 @@
		#ifndef CLAD_NEWTAPE_H
		#define CLAD_NEWTAPE_H

		@@ -0,0 +1,87 @@
		#ifndef CLAD_NEWTAPE_H
		#define CLAD_NEWTAPE_H

Use connected lists for tape data structure #954

Are you sure you want to change the base?

Use connected lists for tape data structure #954

Conversation

rohanjulka19 commented Jun 20, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jun 20, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jun 20, 2024

Choose a reason for hiding this comment

github-actions bot Jun 20, 2024

Choose a reason for hiding this comment

github-actions bot Jun 20, 2024

Choose a reason for hiding this comment

vgvassilev commented Jul 1, 2024

rohanjulka19 commented Jul 1, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 3, 2024

Choose a reason for hiding this comment

github-actions bot Jul 3, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 3, 2024

Choose a reason for hiding this comment

github-actions bot Jul 3, 2024

Choose a reason for hiding this comment

github-actions bot Jul 3, 2024

Choose a reason for hiding this comment

rohanjulka19 commented Jul 4, 2024

vgvassilev commented Jul 4, 2024

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

rohanjulka19 commented Jul 4, 2024

vgvassilev commented Jul 6, 2024

rohanjulka19 commented Jul 6, 2024

vgvassilev commented Jul 19, 2024

rohanjulka19 commented Jul 19, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 24, 2024

Choose a reason for hiding this comment

github-actions bot Jul 24, 2024

Choose a reason for hiding this comment

rohanjulka19 Jul 24, 2024

Choose a reason for hiding this comment

vgvassilev Jul 24, 2024

Choose a reason for hiding this comment

rohanjulka19 commented Jul 24, 2024

vgvassilev Jul 24, 2024

Choose a reason for hiding this comment

rohanjulka19 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

rohanjulka19 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

vgvassilev Jul 24, 2024

Choose a reason for hiding this comment

vgvassilev commented Jul 24, 2024

rohanjulka19 commented Jul 24, 2024 • edited Loading

vgvassilev commented Jul 24, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 12, 2024

Choose a reason for hiding this comment

github-actions bot Sep 12, 2024

Choose a reason for hiding this comment

github-actions bot Sep 12, 2024

Choose a reason for hiding this comment

rohanjulka19 commented Jun 20, 2024 •

edited

Loading

rohanjulka19 commented Jul 1, 2024 •

edited

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

rohanjulka19 commented Jul 19, 2024 •

edited

Loading

rohanjulka19 Jul 24, 2024 •

edited

Loading

rohanjulka19 Jul 24, 2024 •

edited

Loading

rohanjulka19 commented Jul 24, 2024 •

edited

Loading