Bug fixes, documentation (source level), and general cleanup #135

kspangsege · 2013-08-23T11:45:25Z

Correction of detection of changed arrays when updating accessors after Group::commit()
Fixed bug in TableViewBase::sort().
Add free space tracking to a database file that does not have it when calling Group::commit().
In Group::verify(), do not asssert that free space versioning is present just because it is opened in shared mode.
Stop throwing from destructors, and from SharedGroup::rollback() and SharedGRoup::end_read().
Avoid function local statics as they are not necessarily thread-safe.
In GroupWriter::reserve_free_space(), after looking in the last half of the list of free space hunks, look in the first half.
Avoid clobbering the previous database version in GroupWriter::commit() in the non-transactional case.
Eliminate the risk of extending byte size of Group::m_free_positions in GroupWriter::commit() after using its exact size in a calculation.
Eliminate unnecessary free space fragmentation by stopping GroupWriter::reserve_free_space() from adjusting the size of the identified chunk when it is different from the requested size.
GroupWriter::get_fre_space() and Group::Writer::reserve_free_space() now share searching code, and therefore, both skip the first part of the list when searching for a chunk that is larger than 1024 bytes.
Fixed an undetected free-space leak when deleting free-space version tracking in GroupWriter::commit().
More source level documentation.
Many other improvements.

Add free space tracking to a database file that does not have it when calling Group::commit(). In Group::verify(), do not asssert that free space versioning is present just because it is opened in shared mode.

…er Group::commit()

…() in the non-transactional case. Eliminate the risk of extending byte size of Group::m_free_positions in GroupWriter::commit() after using its exact size in a calculation. Eliminate unnecessary free space fragmentation by stopping GroupWriter::reserve_free_space() from adjusting the size of the identified chunk when it is different from the requested size. GroupWriter::get_fre_space() and Group::Writer::reserve_free_space() now share searching code, and therefore, both skip the first part of the list when searching for a chunk that is larger than 1024 bytes. Fixed an undetected free-space leak when deleting free-space version tracking in GroupWriter::commit(). More documentation. Many other improvements.

…f the list of free space hunks, look in the first half

…SharedGRoup::end_read(). Avoid function local statics as they are not necessarily thread-safe.

rrrlasse · 2013-08-23T14:46:05Z

On Windows: test\testshared.cpp:904: error: Failure in Shared_MixedWithNonShared: Unhandled exception: CreateFile() failed: The system cannot find the path specified.

bmunkholm · 2013-08-23T15:05:05Z

test/testshared.cpp

+
+TEST(Shared_MixedWithNonShared)
+{
+    File::try_remove("/tmp/x.tightdb");


/tmp can not be recommended as directory on Windows.
In general I think we should have a define/const for the path, and then use it in general. With this the files will be placed in two locations. For now it would be better to just skip "/tmp/"

Oops. Fixed!

It was never my intention to use a path here. Slipped through from earlier experimentation.

Would it not be possible to configure Visual Studio such that it also runs the test suite with "/test/" as the current working directory?

bmunkholm · 2013-08-23T15:07:19Z

That's a lot.... I recommend Lasse/Alexander review the Array and Group changes. I have not done that in detail.
Best suggestion is to add testcases that reveal the old bugs that are claimed fixed, and now proves they are fixed.

kspangsege · 2013-08-24T15:16:23Z

Test suite passes on Linux and Darwin
Test suite passes on Windows except for the usual issues (Table_test_to_string, Table_test_json_all_data, Table_test_json_simple)
Extended test suite passes (tested on Linux.)
All language binding test suites pass.
No memory leaks in core library (tested on Linux.)
Valgrind reports no new issues (tested on Linux.)
No warnings with GCC 4.6, GCC 4.7, GCC 4.8, Clang 3.2, Clang 4.0.
No new warnings with Visual Studio.
Tested both with and without C++11 enabled (on Linux.)

Note that my tests on Windows are conducted under Virtual Box with zero SSE support enabled.

kspangsege · 2013-08-24T18:03:12Z

Please note that the bulk of the changes revolve around the robustness of Group::commit(), and to a lesser extent, around the robustness of SharedGroup::commit().

In both cases, the robustness is already tested as much as it can be within the context of trivial unit testing. Here is how:

In some cases,the error is already tested for, but occurs very rarely. Example: Correction of detection of changed arrays when updating accessors after Group::commit()
In other cases, the problem could only materialize across an abrupt termination of the process at a critical place. While testing such things is possible, it is far from trivial. Example: Avoid clobbering the previous database version in GroupWriter::commit() in the non-transactional case
In yet other cases, the fixed problem is all about efficiency and performance. Example: Eliminate unnecessary free space fragmentation by stopping GroupWriter::reserve_free_space() from adjusting the size of the identified chunk when it is different from the requested size

…nting the wheel

…eanup

…nting down memory and thread synchronization bugs

astigsen · 2013-08-27T21:33:42Z

src/tightdb/group.hpp

-    return const_cast<Group*>(this)->get_table_ptr<T>(name);
+    TIGHTDB_STATIC_ASSERT(IsBasicTable<T>::value, "Invalid table type");
+    const Table* table = get_table_ptr(name); // Throws
+    TIGHTDB_ASSERT(table || T::matches_dynamic_spec(&table->get_spec()));


Shouldn't it be an && here?

Good find. Actually it should have been "!table || ...". this is fixed in my upcoming commit.

astigsen · 2013-08-27T21:42:10Z

Does not build on Xcode. File 'thread.cpp' is only member of 'libtightdb ios' and not 'libtightdb'. So you get errors from the linker for missing symbols.

Fixing that makes it build, but when trying to run the unit tests, it crashes on startup.

Call stack:

#0 0x000000010022d475 in tightdb::Array::create_empty_array(tightdb::Array::Type, tightdb::Array::WidthType, tightdb::Allocator&) at /Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.cpp:1206
#1 0x00000001004748e2 in tightdb::Array::create_empty_array(tightdb::Array::Type, tightdb::Allocator&) at /Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1576
#2 0x000000010040c248 in tightdb::Array::create(tightdb::Array::Type) at /Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1068
#3 0x000000010047487c in tightdb::Array::Array(tightdb::Array::Type, tightdb::ArrayParent_, unsigned long, tightdb::Allocator&) at /Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1014
#4 0x000000010040c323 in tightdb::Array::Array(tightdb::Array::Type, tightdb::ArrayParent_, unsigned long, tightdb::Allocator&) at /Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1016
#5 0x0000000100038dbe in cxx_global_var_init at /Users/alexanderstigsen/Documents/Dev/tightdb/test/testarray.cpp:291
#6 0x000000010003a7c9 in GLOBAL__I_a at /Users/alexanderstigsen/Documents/Dev/tightdb/test/testarray.cpp:1147
#7 0x00007fff5fc0fda6 in ImageLoaderMachO::doModInitFunctions(ImageLoader::LinkContext const&) ()
#8 0x00007fff5fc0faf2 in ImageLoaderMachO::doInitialization(ImageLoader::LinkContext const&) ()
#9 0x00007fff5fc0d2e4 in ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, ImageLoader::InitializerTimingList&) ()
#10 0x00007fff5fc0e0b7 in ImageLoader::runInitializers(ImageLoader::LinkContext const&, ImageLoader::InitializerTimingList&) ()
#11 0x00007fff5fc034dd in dyld::initializeMainExecutable() ()
#12 0x00007fff5fc0760b in dyld::main(macho_header const, unsigned long, int, char const, char const_, char const_*) ()
#13 0x00007fff5fc01059 in _dyld_start ()

Seems like a linking problem.

…e + some documentation

…was recently eliminated due to problem reports from Valgrind. The problem with not using function local statics, is that the order ot initialization of regular globals is undefined by the C++ standard.

kspangsege · 2013-08-28T01:39:51Z

Problem fixed:

https://github.com/kspangsege/tightdb/commit/62b712fa7353511fb8da0d9ee62393bed6010454

On Tue, Aug 27, 2013 at 11:42 PM, astigsen notifications@github.com wrote:

Does not build on Xcode. File 'thread.cpp' is only member of 'libtightdb
ios' and not 'libtightdb'. So you get errors from the linker for missing
symbols.

Fixing that makes it build, but when trying to run the unit tests, it
crashes on startup.

Call stack:

#0 0x000000010022d475 in
tightdb::Array::create_empty_array(tightdb::Array::Type,
tightdb::Array::WidthType, tightdb::Allocator&) at
/Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.cpp:1206
#1 #1 0x00000001004748e2 in
tightdb::Array::create_empty_array(tightdb::Array::Type,
tightdb::Allocator&) at
/Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1576
#2 #2 0x000000010040c248 in
tightdb::Array::create(tightdb::Array::Type) at
/Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1068
#3 #3 0x000000010047487c in
tightdb::Array::Array(tightdb::Array::Type, tightdb::ArrayParent_, unsigned
long, tightdb::Allocator&) at
/Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1014
#4 #4 0x000000010040c323 in
tightdb::Array::Array(tightdb::Array::Type, tightdb::ArrayParent_, unsigned
long, tightdb::Allocator&) at
/Users/alexanderstigsen/Documents/Dev/tightdb/src/tightdb/array.hpp:1016
#5 #5 0x0000000100038dbe in cxx_global_var_init
at /Users/alexanderstigsen/Documents/Dev/tightdb/test/testarray.cpp:291
#6 #6 0x000000010003a7c9 in
GLOBAL_I_a at
/Users/alexanderstigsen/Documents/Dev/tightdb/test/testarray.cpp:1147
#7 #7 0x00007fff5fc0fda6 in
ImageLoaderMachO::doModInitFunctions(ImageLoader::LinkContext const&) ()
#8 #8 0x00007fff5fc0faf2 in
ImageLoaderMachO::doInitialization(ImageLoader::LinkContext const&) ()
#9 #9 0x00007fff5fc0d2e4 in
ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&,
unsigned int, ImageLoader::InitializerTimingList&) ()
#10 #10 0x00007fff5fc0e0b7 in
ImageLoader::runInitializers(ImageLoader::LinkContext const&,
ImageLoader::InitializerTimingList&) ()
#11 #11 0x00007fff5fc034dd in
dyld::initializeMainExecutable() ()
#12 #12 0x00007fff5fc0760b in
dyld::main(macho_header const, unsigned long, int, char const, char
const__, char const_*) ()
#13 #13 0x00007fff5fc01059 in
_dyld_start ()

Seems like a linking problem.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/135#issuecomment-23373340
.

astigsen · 2013-08-28T04:53:49Z

+1

BTW: Awesome that you included test results in the pull request. That should be done more often.

It would be nice in the future if big changes like that could be broken up in smaller pull requests. Then they could be reviewed faster and the whole process speeded up a bit.

Bug fixes, documentation (source level), and general cleanup

kspangsege · 2013-08-28T11:33:49Z

Finn, I a agree that the idiom would work, but I am reluctant towards introducing Pthread stuff outside the context of shared groups. It would be nice if one could take everything but SharedGroup and compile it on a platform without any threads support.

I think we should look a little further into the thread-safety of function local statics.

Reduce number of threads used on Windows and Android

kspangsege added 13 commits August 15, 2013 15:43

Another small round of cleanup

d565c8c

Use tightdb::Thread API in more places for improved portability

6d23590

Fixed bug in TableViewBase::sort().

b708977

Add free space tracking to a database file that does not have it when calling Group::commit(). In Group::verify(), do not asssert that free space versioning is present just because it is opened in shared mode.

Correction of detection of changed arrays when updating accessors aft…

75d5518

…er Group::commit()

More cleanup

78ada0b

In GroupWriter::reserve_free_space() after looking in the last half o…

6aed1ac

…f the list of free space hunks, look in the first half

Merge branch 'master' into ks-cleanup

e7294cf

Bad line removed from testcase 'Table1'

6d56d96

Merge branch 'master' into ks-cleanup

d8b491a

Stop throwing from destructors, and from SharedGroup::rollback() and …

7b7a37d

…SharedGRoup::end_read(). Avoid function local statics as they are not necessarily thread-safe.

changelog.txt updated

e46d017

Merge branch 'master' into ks-cleanup

7b728d4

ghost assigned bmunkholm Aug 23, 2013

bmunkholm reviewed Aug 23, 2013
View reviewed changes

No longer appempting to place database files in /tmp/

7fc2e4e

kspangsege and others added 10 commits August 25, 2013 15:15

Avoid superflous use of 'const' and use UniquePtr<> instead of reinve…

1bb2b73

…nting the wheel

Typos fixed

2cd8f48

Use ScopedMutexLock when possible, and keep track of exception sources

298188d

Merge branch 'ks-cleanup' of github.com:kspangsege/tightdb into ks-cl…

9666623

…eanup

New conditional compilation flag TIGHTDB_ALLOC_SET_ZERO to help in hu…

dfc3700

…nting down memory and thread synchronization bugs

bugfix: remap lock file during end_read if necessary

4ffc6b6

Merge branch 'master' into ks-cleanup

bc7e2c9

Merge branch 'master' into ks-cleanup

8fa0f1d

Merge branch 'fs-ringbuf_fix' into ks-cleanup

204e6e5

Merge branch 'master' into ks-node-consolidation

0cd7d26

kspangsege added 2 commits August 27, 2013 13:09

Merge branch 'ks-cleanup' into ks-node-consolidation

99b111b

A tiny bit of cleanup

9ce762e

kspangsege mentioned this pull request Aug 27, 2013

Compact B+-tree nodes (Node Consolidation) #138

Closed

astigsen reviewed Aug 27, 2013
View reviewed changes

kspangsege added 3 commits August 28, 2013 00:36

Fixing wrong assertion in Group::get_table_ptr() and adding a few mor…

71405a0

…e + some documentation

Fixing Xcode project

6e705da

Reintroducing use of function local static for default allocator. It …

62b712f

…was recently eliminated due to problem reports from Valgrind. The problem with not using function local statics, is that the order ot initialization of regular globals is undefined by the C++ standard.

kspangsege added a commit that referenced this pull request Aug 28, 2013

Merge pull request #135 from kspangsege/ks-cleanup

6a3fda1

Bug fixes, documentation (source level), and general cleanup

kspangsege merged commit 6a3fda1 into realm:master Aug 28, 2013

kspangsege deleted the ks-cleanup branch August 28, 2013 09:42

tgoyne pushed a commit that referenced this pull request Jul 11, 2018

Merge pull request #135 from realm/jed/ci-improvements

aaa730b

Reduce number of threads used on Windows and Android

gvravikumar mentioned this pull request Oct 30, 2023

Failure to open a Realm file on Android 14 #7037

Open

sync-by-unito bot mentioned this pull request Feb 16, 2024

App crashing when starting Realm: [realm-core-11.0.2] Assertion failed: r == 0 && "File::unlock() " ... #4777

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes, documentation (source level), and general cleanup #135

Bug fixes, documentation (source level), and general cleanup #135

kspangsege commented Aug 23, 2013

rrrlasse commented Aug 23, 2013

bmunkholm Aug 23, 2013

kspangsege Aug 24, 2013

bmunkholm commented Aug 23, 2013

kspangsege commented Aug 24, 2013

kspangsege commented Aug 24, 2013

astigsen Aug 27, 2013

kspangsege Aug 27, 2013

astigsen commented Aug 27, 2013

kspangsege commented Aug 28, 2013

astigsen commented Aug 28, 2013

kspangsege commented Aug 28, 2013

Bug fixes, documentation (source level), and general cleanup #135

Bug fixes, documentation (source level), and general cleanup #135

Conversation

kspangsege commented Aug 23, 2013

rrrlasse commented Aug 23, 2013

bmunkholm Aug 23, 2013

Choose a reason for hiding this comment

kspangsege Aug 24, 2013

Choose a reason for hiding this comment

bmunkholm commented Aug 23, 2013

kspangsege commented Aug 24, 2013

kspangsege commented Aug 24, 2013

astigsen Aug 27, 2013

Choose a reason for hiding this comment

kspangsege Aug 27, 2013

Choose a reason for hiding this comment

astigsen commented Aug 27, 2013

kspangsege commented Aug 28, 2013

astigsen commented Aug 28, 2013

kspangsege commented Aug 28, 2013