StringEnumColumn testing #2780

ironage · 2017-08-01T23:52:45Z

There was one bug introduced when the swap_row instruction was implemented: StringEnumColumn was forgotten so the default Column implementation is called. What ends up happening is we set a StringData payload into an IntegerColumn type bp-tree. The fix is simply implementing StringEnumColumn::swap_rows() just like StringColumn does it. This fixes #2766 (and adds the test there).

The rest of the changes increase coverage of the StringEnumColumn type by making use of the TEST_TYPES macro and some custom test structs implemented in test/test_string_types.hpp. I found that I couldn't see the test headers in the generated Xcode project, so I added them to the project via the CMakeLists.txt config.

The increased number of tests increased the test time too much, so I reduced the number of iterations in the biggest offender StringIndex_Insensitive_Fuzz.

Before:

Success: All 1114 tests passed (18058128 checks).
Test time: 1m8s
Top 5 time usage:
-----------------------------------------------------------------------------
StringIndex_Insensitive_Fuzz<std::__1::integral_constant<bool,+true>>   7.28s
StringIndex_Insensitive_Fuzz<std::__1::integral_constant<bool,+false>>  6.67s
ThreadSafety_RowDestruction                                             5.89s
Shared_StaticFuzzTestRunSanityCheck                                     5.06s
ThreadSafety_LinkViewDestruction                                        3.66s

After:

Success: All 1203 tests passed (15787255 checks).
Test time: 1m4s
Top 5 time usage:
------------------------------------------------
ThreadSafety_RowDestruction                6.14s
ThreadSafety_TableViewDestruction          4.24s
ThreadSafety_LinkViewDestruction           2.85s
StringIndex_Insensitive_Fuzz<enum_column>  2.70s
Shared_ManyReaders                         2.57s

realm-ci · 2017-08-01T23:58:59Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/1/Diff_Coverage

realm-ci · 2017-08-02T00:00:09Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/1/Performance_Report

realm-ci · 2017-08-02T03:35:10Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/2/Performance_Report

realm-ci · 2017-08-02T03:40:18Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/2/Diff_Coverage

realm-ci · 2017-08-02T23:02:37Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/3/Diff_Coverage

realm-ci · 2017-08-02T23:03:58Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/3/Performance_Report

ironage · 2017-08-02T23:51:10Z

I've cherry-picked from #2778 because that needs to go into master, otherwise CI fails this PR.

rrrlasse · 2017-08-07T08:23:32Z

src/realm/column_string_enum.cpp

@@ -162,6 +162,39 @@ void StringEnumColumn::do_move_last_over(size_t row_ndx, size_t last_row_ndx)
    move_last_over_without_updating_index(row_ndx, last_row_ndx); // Throws
 }

+void StringEnumColumn::swap_rows(size_t row_ndx_1, size_t row_ndx_2)
+{
+    REALM_ASSERT_3(row_ndx_1, <=, size());


Shouldn't this be < instead of <=?

rrrlasse · 2017-08-07T08:36:24Z

src/realm/column_string_enum.cpp

+        set(row_ndx_2, realm::null());
+    }
+    else {
+        StringData copy{buffer_1.data(), buffer_1.size()};


You can just swap the integers with IntegerColumn::get() / IntegerColumn::set() instead of copying payload

rrrlasse · 2017-08-07T08:38:47Z

test/fuzz_group.cpp

@@ -207,7 +207,7 @@ Mixed construct_mixed(State& s, util::Optional<std::ostream&> log, std::string&
        }
        case 5: {
            size_t rand_char = get_next(s);
-            size_t blob_size = get_int64(s) % ArrayBlob::max_binary_size;
+            size_t blob_size = static_cast<uint64_t>(get_int64(s)) % ArrayBlob::max_binary_size;


You should do % (ArrayBlob::max_binary_size + 1) to be able to also trigger the maximum blob size, which is an important edge case

ironage · 2017-08-09T21:22:16Z

@rrrlasse I've made the optimisation in StringEnumColumn::swap_rows can you please review it for correctness and efficiency again? (commit a3545aa)

realm-ci · 2017-08-09T21:26:14Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/5/Performance_Report

realm-ci · 2017-08-09T21:28:10Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/5/Diff_Coverage

realm-ci · 2017-08-11T23:26:32Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/6/Diff_Coverage

realm-ci · 2017-08-11T23:27:22Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/6/Performance_Report

ironage · 2017-08-17T19:07:58Z

@finnschiermer @rrrlasse I've been running the fuzz tests on this branch for several days without finding any new crashes. I believe the StringEnumColumn to be stable. Please review when you have the time.

js@ubuntu-linux32bit:~/Documents/code/realm-core/build.afl$ afl-whatsup findings/
status check tool for afl-fuzz by <lcamtuf@google.com>

Individual fuzzers
==================

>>> fuzzer1 (2 days, 22 hrs) <<<

  cycle 1, lifetime speed 26 execs/sec, path 4036/17369 (23%)
  pending 1033/17185, coverage 39.13%, no crashes yet

>>> fuzzer2 (2 days, 22 hrs) <<<

  cycle 1, lifetime speed 11 execs/sec, path 2043/19539 (10%)
  pending 1093/19469, coverage 39.15%, no crashes yet

Summary stats
=============

       Fuzzers alive : 2
      Total run time : 5 days, 21 hours
         Total execs : 9 million
    Cumulative speed : 38 execs/sec
       Pending paths : 2126 faves, 36654 total
  Pending per fuzzer : 1063 faves, 18327 total (on average)
       Crashes found : 0 locally unique

ironage · 2017-08-18T00:11:14Z

Looks like I was a bit too hasty there, at the 10 million execution point AFL found some crashes in StringEnumColumn (not related to swap_rows) I'm investigating.

ironage · 2017-08-18T23:35:04Z

The fuzzer also found that Table::move_column() was broken when moving string enum columns around. It is fixed in ef46b28. I'll continue fuzz tests as a background task but these changes are ready for review.

realm-ci · 2017-08-19T00:23:48Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/8/Diff_Coverage

realm-ci · 2017-08-19T00:24:38Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/8/Performance_Report

realm-ci · 2017-08-22T19:30:56Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/9/Diff_Coverage

realm-ci · 2017-08-22T19:42:48Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/9/Performance_Report

finnschiermer

Very nice.

finnschiermer · 2017-08-28T15:08:28Z

src/realm/column_string_enum.cpp

+    }
+
+    // Update search index
+    // (it is important here that we do it before actually setting


ironage · 2017-09-06T23:23:20Z

@rrrlasse did you get a chance to look at the changes here or is it good to go?

ironage · 2017-09-13T21:00:31Z

@rrrlasse bump for review

realm-ci · 2017-09-13T21:08:14Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/10/Diff_Coverage

realm-ci · 2017-09-13T21:13:51Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/10/Performance_Report

rrrlasse

See comments

rrrlasse · 2017-09-15T09:59:20Z

src/realm/column_string_enum.cpp

+{
+    REALM_ASSERT_3(row_ndx_1, <, size());
+    REALM_ASSERT_3(row_ndx_2, <, size());
+    REALM_ASSERT_DEBUG(row_ndx_1 != row_ndx_2);


Do all other column types use != also?

Yes other columns have the same asserts, in particular StringColumn.

rrrlasse · 2017-09-15T10:40:39Z

test/fuzz_group.cpp

@@ -678,7 +678,7 @@ void parse_and_apply_instructions(std::string& in, const std::string& path, util
                            bool insert_big_blob = get_next(s) % 2 == 0;
                            if (insert_big_blob) {
                                size_t rand_char = get_next(s);
-                                size_t blob_size = get_next(s) + ArrayBlob::max_binary_size;
+                                size_t blob_size = get_next(s) + (ArrayBlob::max_binary_size + 1);


Shouldn't this be static_cast<uint64_t>(get_int64(s)) % (ArrayBlob::max_binary_size + 1);? Or better yet, is there a reason for not reusing the existing generator in construct_mixed()?

Good idea, updated to share the correct ™ binary creation code in a function.

realm-ci · 2017-09-15T19:13:13Z

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/11/Diff_Coverage

realm-ci · 2017-09-15T19:16:29Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2780/11/Performance_Report

ironage added S:Review T-Bug-Crash labels Aug 1, 2017

ironage self-assigned this Aug 1, 2017

ironage requested review from bdash, rrrlasse and finnschiermer August 1, 2017 23:52

rrrlasse approved these changes Aug 7, 2017

View reviewed changes

James Stone added 6 commits August 9, 2017 14:18

Fix StringEnumColumn swap_rows

9b41f90

Increase string enums test coverage

bcaaef0

Make test sources viewable in cmake generated projects

f6740f5

Add note to the change log

ae312c1

Optimize StringEnumColumn::swap_rows

a3545aa

Allow max binary size blobs in fuzz tests

e41fc1a

ironage force-pushed the js/string-enum-fixes branch from 71dd478 to e41fc1a Compare August 9, 2017 21:20

Further optimisation and fix to swap_rows

72bda0c

ironage mentioned this pull request Aug 18, 2017

Optimize in a write transaction realm/realm-object-store#501

Closed

Fix moving string enum columns

2c9c41c

ironage force-pushed the js/string-enum-fixes branch from ef46b28 to 2c9c41c Compare August 22, 2017 19:16

bmunkholm removed the S:Review label Aug 23, 2017

finnschiermer approved these changes Aug 28, 2017

View reviewed changes

Merge branch 'master' into js/string-enum-fixes

b5806bd

rrrlasse suggested changes Sep 15, 2017

View reviewed changes

James Stone added 2 commits September 15, 2017 12:01

Reuse binary creation code in fuzz test

6381d13

Merge branch 'master' into js/string-enum-fixes

d80b0c4

rrrlasse approved these changes Sep 18, 2017

View reviewed changes

ironage merged commit 1395515 into master Sep 18, 2017

ironage deleted the js/string-enum-fixes branch September 18, 2017 14:37

github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2024

StringEnumColumn testing #2780

StringEnumColumn testing #2780

Conversation

ironage commented Aug 1, 2017 • edited Loading

realm-ci commented Aug 1, 2017

realm-ci commented Aug 2, 2017

realm-ci commented Aug 2, 2017

realm-ci commented Aug 2, 2017

realm-ci commented Aug 2, 2017

realm-ci commented Aug 2, 2017

ironage commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironage commented Aug 9, 2017

realm-ci commented Aug 9, 2017

realm-ci commented Aug 9, 2017

realm-ci commented Aug 11, 2017

realm-ci commented Aug 11, 2017

ironage commented Aug 17, 2017

ironage commented Aug 18, 2017

ironage commented Aug 18, 2017

realm-ci commented Aug 19, 2017

realm-ci commented Aug 19, 2017

realm-ci commented Aug 22, 2017

realm-ci commented Aug 22, 2017

finnschiermer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironage commented Sep 6, 2017

ironage commented Sep 13, 2017

realm-ci commented Sep 13, 2017

realm-ci commented Sep 13, 2017

rrrlasse 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

realm-ci commented Sep 15, 2017

realm-ci commented Sep 15, 2017

ironage commented Aug 1, 2017 •

edited

Loading