No elide constructors #7798

mrambacher · 2020-12-22T01:57:07Z

Added "no-elide-constructors to the ASSERT_STATUS_CHECK builds. This flag gives more errors/warnings for some of the Status checks where an inner class checks a Status and later returns it. In this case, without the elide check on, the returned status may not have been checked in the caller, thereby bypassing the checked code.

pdillinger

Otherwise, 👍 👍 👍

pdillinger · 2020-12-22T16:47:08Z

Makefile

@@ -190,6 +190,8 @@ else
 endif

 ifdef ASSERT_STATUS_CHECKED
+# For ASC, turn off constructor elision


This "what" comment doesn't tell me anything I can't reverse engineer from about 3 seconds of reading the Makefile. Tell me why.

I can't even turn it off and see what breaks, because it only works to prevent regressions.

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pdillinger

And four clang-analyze false positive "pointer is NULL" reports to suppress. I don't think we have a better way than what is shown in 99f5a80 (though a comment for why assert(db != nullptr) is preferred, such as // suppress false clang-analyze report).

facebook-github-bot · 2020-12-22T22:29:25Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

pdillinger · 2020-12-22T23:59:46Z

db/corruption_test.cc

@@ -616,7 +616,7 @@ TEST_F(CorruptionTest, ParanoidFileChecksOnFlush) {
    options.table_factory = mock;
    mock->SetCorruptionMode(mode);
    ASSERT_OK(DB::Open(options, dbname_, &db_));
-    assert(db_ != nullptr);


These asserts were put there for clang analyzer (without an informative comment 😢 ) and changing them leads to return of the false reports.

facebook-github-bot · 2020-12-23T02:17:31Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-12-23T03:08:48Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-12-23T18:10:22Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

pdillinger

Awesomeness. Thanks!

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-12-24T01:37:34Z

@pdillinger merged this pull request in 55e9968.

siying · 2020-12-30T02:29:34Z

db/db_impl/db_impl_compaction_flush.cc

@@ -627,7 +627,7 @@ Status DBImpl::AtomicFlushMemTablesToOutputFiles(
      if (sfm) {
        std::string file_path = MakeTableFileName(
            cfds[i]->ioptions()->cf_paths[0].path, file_meta[i].fd.GetNumber());
-        sfm->OnAddFile(file_path);
+        s = sfm->OnAddFile(file_path);


Stress test now sometimes shows a problem and I suspect that this line is related. Here is the failure:

2020/12/26-14:41:51.516292 14b0efeeb700 [ERROR] [/db_impl/db_impl_compaction_flush.cc:2521] Waiting after background flush error: IO error: No such file or directorywhile stat a file for size: /dev/shm/rocksdb/rocksdb_c rashtest_whitebox/000000.sst: No such file or directoryAccumulated background error counts: 1

My theory is that, in the case where the memtable is empty, there is no file flushed but here the registration is called anyway. It used to be that sfm->OnAddFile() rejected it since the file doesn't exist, but now it fails the flush, which is unexpected. If we agree that it is the problem, we should fix it.

@siying There is a comparable line above (line 287). The errors could be ignored if required or is there a way to test that the file is not empty/was flushed? How do we write a test to show this failure?

If atomic flush is enabled, SFM is enabled, and there are more than one CF with all of their memtables to be empty, a Flush() call might trigger this issue.

A better fix might be that if no file is generated, we should not go into this code path. We probably can know whether a file is created (file_meta[i] is ever updated) in some way.

…lideConstructors

mrambacher · 2020-12-31T16:24:33Z

Push a change to revert the checks on SFM for stress test failures.

facebook-github-bot · 2020-12-31T16:25:10Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-12-31T19:34:20Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

siying · 2020-12-31T20:37:53Z

db/db_impl/db_impl_compaction_flush.cc

+        // TODO (PR7798).  We should only add the file to the FileManager if it
+        // exists. Otherwise, some tests may fail.  Ignore the error in the
+        // interim.
+        sfm->OnAddFile(file_path).PermitUncheckedError();


Thanks for the fix. It looks good but I think we should create a new PR to fix it, rather than changing an existing one.

mrambacher · 2021-01-03T21:08:13Z

Split the ignore for the SSTFileManager::OnAddFile into #7826

Summary: Added "no-elide-constructors to the ASSERT_STATUS_CHECK builds. This flag gives more errors/warnings for some of the Status checks where an inner class checks a Status and later returns it. In this case, without the elide check on, the returned status may not have been checked in the caller, thereby bypassing the checked code. Pull Request resolved: facebook#7798 Reviewed By: jay-zhuang Differential Revision: D25680451 Pulled By: pdillinger fbshipit-source-id: c3f14ed9e2a13f0a8c54d839d5fb4d1fc1e93917

Summary: Added "no-elide-constructors to the ASSERT_STATUS_CHECK builds. This flag gives more errors/warnings for some of the Status checks where an inner class checks a Status and later returns it. In this case, without the elide check on, the returned status may not have been checked in the caller, thereby bypassing the checked code. Pull Request resolved: facebook/rocksdb#7798 Reviewed By: jay-zhuang Differential Revision: D25680451 Pulled By: pdillinger fbshipit-source-id: c3f14ed9e2a13f0a8c54d839d5fb4d1fc1e93917 Signed-off-by: Changlong Chen <levisonchen@live.cn>

mrambacher added 4 commits December 10, 2020 01:40

Fix ASSERT_STATUS_CHECK for copy elision

9be4eeb

Merge branch 'master' into NoElideConstructors

f65fa09

Merge branch 'master' into NoElideConstructors

8e287dd

Merge branch 'master' into NoElideConstructors

b28f185

mrambacher requested review from ajkr and pdillinger December 22, 2020 01:57

facebook-github-bot added the CLA Signed label Dec 22, 2020

pdillinger approved these changes Dec 22, 2020

View reviewed changes

facebook-github-bot reviewed Dec 22, 2020

View reviewed changes

pdillinger requested changes Dec 22, 2020

View reviewed changes

Updated no-elide comment

673aa4c

pdillinger reviewed Dec 22, 2020

View reviewed changes

Address clang-analyze failure reports

db6033f

mrambacher force-pushed the NoElideConstructors branch from 037bace to db6033f Compare December 23, 2020 03:08

Merge branch 'master' into NoElideConstructors

07c9862

pdillinger approved these changes Dec 23, 2020

View reviewed changes

facebook-github-bot reviewed Dec 23, 2020

View reviewed changes

facebook-github-bot closed this in 55e9968 Dec 24, 2020

facebook-github-bot added the Merged label Dec 24, 2020

siying reviewed Dec 30, 2020

View reviewed changes

Merge branch 'master' of https://github.com/facebook/rocksdb into NoE…

1f944fa

…lideConstructors

mrambacher reopened this Dec 31, 2020

Update db_impl_compaction_flush.cc

b8c5322

mrambacher force-pushed the NoElideConstructors branch from 6b32313 to b8c5322 Compare December 31, 2020 19:34

siying reviewed Dec 31, 2020

View reviewed changes

mrambacher closed this Jan 3, 2021

mrambacher deleted the NoElideConstructors branch March 29, 2021 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No elide constructors #7798

No elide constructors #7798

mrambacher commented Dec 22, 2020

pdillinger left a comment

pdillinger Dec 22, 2020

facebook-github-bot left a comment

pdillinger left a comment

facebook-github-bot commented Dec 22, 2020

pdillinger Dec 22, 2020

facebook-github-bot commented Dec 23, 2020

facebook-github-bot commented Dec 23, 2020

facebook-github-bot commented Dec 23, 2020

pdillinger left a comment

facebook-github-bot left a comment

facebook-github-bot commented Dec 24, 2020

siying Dec 30, 2020

mrambacher Dec 30, 2020

siying Dec 30, 2020

mrambacher commented Dec 31, 2020

facebook-github-bot commented Dec 31, 2020

facebook-github-bot commented Dec 31, 2020

siying Dec 31, 2020

mrambacher commented Jan 3, 2021

No elide constructors #7798

No elide constructors #7798

Conversation

mrambacher commented Dec 22, 2020

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger Dec 22, 2020

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

pdillinger left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 22, 2020

pdillinger Dec 22, 2020

Choose a reason for hiding this comment

facebook-github-bot commented Dec 23, 2020

facebook-github-bot commented Dec 23, 2020

facebook-github-bot commented Dec 23, 2020

pdillinger left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 24, 2020

siying Dec 30, 2020

Choose a reason for hiding this comment

mrambacher Dec 30, 2020

Choose a reason for hiding this comment

siying Dec 30, 2020

Choose a reason for hiding this comment

mrambacher commented Dec 31, 2020

facebook-github-bot commented Dec 31, 2020

facebook-github-bot commented Dec 31, 2020

siying Dec 31, 2020

Choose a reason for hiding this comment

mrambacher commented Jan 3, 2021