[FIX] Fixes aligned allocator issue for non posix implementation of aligned_alloc. #1362

rrahn · 2019-11-07T13:40:01Z

Uses operator new and delete instead of aligned_alloc.

codecov · 2019-11-07T14:30:05Z

Codecov Report

Merging #1362 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1362      +/-   ##
==========================================
+ Coverage   97.51%   97.55%   +0.03%     
==========================================
  Files         224      226       +2     
  Lines        8731     8781      +50     
==========================================
+ Hits         8514     8566      +52     
+ Misses        217      215       -2

Impacted Files	Coverage Δ
...clude/seqan3/range/container/aligned_allocator.hpp	`100% <100%> (+18.18%)`	⬆️
include/seqan3/alphabet/aminoacid/aa10li.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/alphabet/mask/masked.hpp	`100% <0%> (ø)`	⬆️
.../seqan3/alphabet/composite/alphabet_tuple_base.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/alphabet/concept.hpp	`100% <0%> (ø)`	⬆️
...ude/seqan3/alphabet/composite/alphabet_variant.hpp	`100% <0%> (ø)`	⬆️
...nclude/seqan3/alphabet/structure/structured_aa.hpp	`77.77% <0%> (ø)`	⬆️
...clude/seqan3/alphabet/aminoacid/aminoacid_base.hpp	`100% <0%> (ø)`	⬆️
.../pairwise/execution/alignment_executor_two_way.hpp	`98.33% <0%> (ø)`	⬆️
include/seqan3/alphabet/cigar/cigar.hpp	`90.47% <0%> (ø)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fa069f...e444f99. Read the comment docs.

marehr · 2019-11-13T14:29:35Z

include/seqan3/range/container/aligned_allocator.hpp

     */
    template <typename new_value_type>
    struct rebind
    {
+        //!\brief The alignment for the rebound allocator.
+        static constexpr size_t other_alignment = (alignof(new_value_type) > alignment)


marehr

some questions and some suggestion and some love

marehr · 2019-11-13T14:38:03Z

test/unit/range/container/aligned_allocator_test.cpp

@@ -80,6 +80,68 @@ TEST(aligned_allocator, memory_alignment)
    alloc.deallocate(begin, size);
 }

+TEST(aligned_allocator, memory_alignment_new_extended)


What does new_extended mean?

this is language from the c++ draft. It means that it's alignment is bigger than the value of __STDCPP_DEFAULT_NEW_ALIGNMENT__

marehr · 2019-11-13T14:38:31Z

test/unit/range/container/aligned_allocator_test.cpp

+    alignas(64) std::array<int32_t, 2> data{};
+};
+
+TEST(aligned_allocator, memory_alignment_new_extended_default)


What does new_extended_default mean?

marehr · 2019-11-13T14:39:12Z

test/unit/range/container/aligned_allocator_test.cpp

+    EXPECT_EQ(memory_alignment(begin, alignment), 0u);
+    EXPECT_EQ(memory_alignment(end,   alignment), 0u);
+
+    EXPECT_EQ(memory_alignment(begin + 1,  alignment), 0u);


Why is it always 0? I don't want to think :)

because it does not use a larger alignment than the alignment of the type that is allocated for (which is 64 bytes). So every value starts at 64 byte boundary. Also that's why I called this test default to show the behavior of a new extended type where we use the default alignment of the type and not a larger one.

marehr · 2019-11-13T15:01:18Z

include/seqan3/range/container/aligned_allocator.hpp

+ * This class allocates memory at the given alignment boundary. If the specified `alignment` is not supported by the
+ * used allocation method a std::bad_alloc exception will be thrown. For requested alignments larger than
+ * `__STDCPP_DEFAULT_NEW_ALIGNMENT__` the storage will have the alignment specified by the value `alignment`.
+ * Otherwise, the storage is aligned for any object that does not have new-extended alignment and is of the requested


What is new-extended? Can you link it to some cppreference page?

And is the other one not of the requested size?

Can you give some examples which "objects" might be effected by the else case?

like everything up to __STDCPP_DEFAULT_NEW_ALIGNMENT__ bytes. IIRC that is 16 byte. But I look into this and extend the example with some comments. I guess it shows this already.

include/seqan3/range/container/aligned_allocator.hpp

marehr · 2019-11-13T15:36:02Z

include/seqan3/range/container/aligned_allocator.hpp

+     * [__STDCPP_DEFAULT_NEW_ALIGNMENT__](https://en.cppreference.com/w/cpp/memory/new/align_val_t) the alignment
+     * aware operator new that takes as second argument the alignment as std::align_val_t. The allocation methods
+     * used by the global operator new functions is unspecified, e.g. std::malloc or std::aligned_alloc. Accordingly,
+     * the requirements on the alignment and the size of allocated bytes is implementation defined.


Does this has any impact on the intended behaviour of this class?

I don't understand you question. Could you be more specific? I mean this is the same problem when to use std::aligned_alloc. Depending on how operator new is implemented and depending on the platform different things are possible. So for example it might still be a problem in the future, that on an architecture that uses posix mem_align arbitrary sizes of bytes to allocate can be used, while on others this might be restricted to sizes that are a multiple of the alignment. But that is implementation defined and beyond the scope of this class. It only makes sure that it calls aligned memory, where it uses whatever ::operator new(n) uses to align up to alignments of __STDCPP_DEFAULT_NEW_ALIGNMENT__ and beyond that aligns the memory depending on whatever ::operator new(n, alignment) uses.

Note: We call the new operator with the semantic requirements that the c++ standard specifies/demands, but be aware that users can overload any (global) ::operator new that might not adhere to the standard and might cause std::bad_alloc or unaligned pointers.

include/seqan3/range/container/aligned_allocator.hpp

…ligned_alloc. * Uses operator new and delete instead.

marehr

Looks great! One thing left.

smehringer · 2019-12-03T12:08:20Z

include/seqan3/range/container/aligned_allocator.hpp

@@ -28,6 +28,12 @@ namespace seqan3
 *
 * \details
 *
+ * This class allocates memory at the given alignment boundary. If the specified `alignment` is not supported by the


can you always use the term memory-alignment instead of just alignment like in the documentation below? It helps to know up front that this has nothing to do with our pairwise alignment

I will add a section about what memory alignment is.

include/seqan3/range/container/aligned_allocator.hpp

smehringer · 2019-12-03T12:20:33Z

include/seqan3/range/container/aligned_allocator.hpp

+ * used allocation method a std::bad_alloc exception will be thrown. For requested alignments larger than
+ * `__STDCPP_DEFAULT_NEW_ALIGNMENT__`, also called new-extended alignments, the storage will have the alignment
+ * specified by the value `alignment`. Otherwise, the storage is aligned for any object that does not have new-extended
+ * alignment, e.g. int or double, and is of the requested size.


From this text, I do not understand what a memory alignment is.
If you think this is common knowledge, can you give just me an explanation? If not, one or few sentences would be great since this is a public interface.

Side note: I see that the number is always a multiple of the word size. Is this required and if so do you static_assert this?

Side note: I see that the number is always a multiple of the word size. Is this required and if so do you static_assert this?

I am not sure I understand what do you mean?

smehringer · 2019-12-03T12:23:55Z

include/seqan3/range/container/aligned_allocator.hpp

+     * [operator new](https://en.cppreference.com/w/cpp/memory/new/operator_new).
+     * If the given `alignment` is bigger than
+     * [__STDCPP_DEFAULT_NEW_ALIGNMENT__](https://en.cppreference.com/w/cpp/memory/new/align_val_t) the alignment
+     * aware operator new that takes as second argument the alignment as std::align_val_t.


I think this sentence is mal formatted... at least I cannot make sense of it

smehringer · 2019-12-03T12:27:57Z

include/seqan3/range/container/aligned_allocator.hpp

     * \details
     *
-     * The argument n must be equal to the first argument of the call to allocate() that originally produced p;
-     * otherwise, the behavior is undefined.
+     * The argument `n` must be equal to the first argument of the call to seqan3::aligned_allocator::allocate that


This seems error prone? Can't we cache n and remove the parameter from this function?

This is defined and documented according to the allocator interface requirements: https://en.cppreference.com/w/cpp/named_req/Allocator, and I am afraid we need to keep this interface.

smehringer · 2019-12-03T12:29:04Z

test/unit/range/container/aligned_allocator_test.cpp

@@ -47,6 +48,12 @@ TEST(aligned_allocator, conversion_constructor)
    [[maybe_unused]] aligned_allocator<float, 16> float_alloc{int_alloc};
 }

+TEST(aligned_allocator, request_to_much_memory)


Suggested change

TEST(aligned_allocator, request_to_much_memory)

TEST(aligned_allocator, request_too_much_memory)

💅

…ligned_alloc. (seqan#1362) * [FIX] Fixes aligned allocator issue for non posix implementation of aligned_alloc. * Uses operator new and delete instead.

rrahn requested a review from marehr November 7, 2019 13:40

rrahn mentioned this pull request Nov 7, 2019

[INFRA] Enable catalina g++-9 builds. #1296

Merged

rrahn force-pushed the fix/allocator_bug branch 2 times, most recently from 79ed7ce to 1dd9b31 Compare November 8, 2019 09:23

marehr reviewed Nov 13, 2019

View reviewed changes

include/seqan3/range/container/aligned_allocator.hpp Show resolved Hide resolved

marehr reviewed Nov 13, 2019

View reviewed changes

include/seqan3/range/container/aligned_allocator.hpp Show resolved Hide resolved

[FIX] Fixes aligned allocator issue for non posix implementation of a…

90dfea7

…ligned_alloc. * Uses operator new and delete instead.

rrahn force-pushed the fix/allocator_bug branch from 1dd9b31 to 90dfea7 Compare November 15, 2019 16:47

rrahn requested a review from marehr November 15, 2019 16:47

rrahn mentioned this pull request Nov 24, 2019

Feature/compute vectorised score for global alignment #1379

Merged

marehr approved these changes Nov 27, 2019

View reviewed changes

applying code review comments

1e861a7

rrahn requested a review from smehringer November 30, 2019 15:05

marehr approved these changes Nov 30, 2019

View reviewed changes

smehringer requested changes Dec 3, 2019

View reviewed changes

Apply review comments

ce38b48

rrahn requested a review from smehringer December 3, 2019 15:06

fix small documentation issues

e444f99

h-2 mentioned this pull request Dec 4, 2019

use _aligned_malloc to implement aligned_allocator in _WIN32 #1288

Closed

smehringer approved these changes Dec 5, 2019

View reviewed changes

smehringer merged commit cfcc96e into seqan:master Dec 5, 2019

rrahn deleted the fix/allocator_bug branch January 16, 2020 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Fixes aligned allocator issue for non posix implementation of aligned_alloc. #1362

[FIX] Fixes aligned allocator issue for non posix implementation of aligned_alloc. #1362

rrahn commented Nov 7, 2019

codecov bot commented Nov 7, 2019 •

edited

Loading

marehr Nov 13, 2019

marehr left a comment

marehr Nov 13, 2019

rrahn Nov 14, 2019

marehr Nov 13, 2019

marehr Nov 13, 2019

rrahn Nov 14, 2019

marehr Nov 13, 2019

marehr Nov 13, 2019

marehr Nov 13, 2019

rrahn Nov 14, 2019

marehr Nov 13, 2019

rrahn Nov 14, 2019

marehr Nov 27, 2019 •

edited

Loading

marehr left a comment

smehringer Dec 3, 2019

rrahn Dec 3, 2019

smehringer Dec 3, 2019

rrahn Dec 3, 2019

smehringer Dec 3, 2019

smehringer Dec 3, 2019

rrahn Dec 3, 2019

smehringer Dec 3, 2019

	TEST(aligned_allocator, request_to_much_memory)
	TEST(aligned_allocator, request_too_much_memory)

[FIX] Fixes aligned allocator issue for non posix implementation of aligned_alloc. #1362

[FIX] Fixes aligned allocator issue for non posix implementation of aligned_alloc. #1362

Conversation

rrahn commented Nov 7, 2019

codecov bot commented Nov 7, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

marehr left a 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

Choose a reason for hiding this comment

marehr Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

marehr left a 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

codecov bot commented Nov 7, 2019 •

edited

Loading

marehr Nov 27, 2019 •

edited

Loading