Skip to content

Commit 6a5444b

Browse files
committed
Implement Reference Counted Memory Handling
1 parent bbe87e4 commit 6a5444b

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

68 files changed

+5737
-4541
lines changed

Makefile.conf

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ SOURCES = \
4444
to_value.cpp \
4545
source_map.cpp \
4646
error_handling.cpp \
47-
memory_manager.cpp \
47+
memory/SharedPtr.cpp \
4848
utf8_string.cpp \
4949
base64vlq.cpp
5050

appveyor.yml

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ os: Visual Studio 2013
33
environment:
44
CTEST_OUTPUT_ON_FAILURE: 1
55
ruby_version: 22-x64
6-
TargetPath: sassc/bin/sassc
6+
TargetPath: sassc/bin/sassc.exe
77
matrix:
88
- Compiler: msvc
99
Config: Release
@@ -63,14 +63,19 @@ test_script:
6363
git -C sass-spec checkout -q --force ci-spec-pr-$SPEC_PR
6464
}
6565
}
66-
ruby sass-spec/sass-spec.rb -V 3.4 --probe-todo --impl libsass -c $env:TargetPath -s sass-spec/spec
67-
if(-not($?)) {
68-
echo "sass-spec tests failed"
66+
$env:TargetPath = Join-Path $pwd.Path $env:TargetPath
67+
If (Test-Path "$env:TargetPath") {
68+
ruby sass-spec/sass-spec.rb -V 3.4 --probe-todo --impl libsass -c $env:TargetPath -s sass-spec/spec
69+
if(-not($?)) {
70+
echo "sass-spec tests failed"
71+
exit 1
72+
}
73+
} else {
74+
echo "spec runner not found (compile error?)"
6975
exit 1
7076
}
7177
Write-Host "Explicitly testing the case when cwd has Cyrillic characters: " -nonewline
7278
# See comments in gh-1774 for details.
73-
$env:TargetPath = Join-Path $pwd.Path $env:TargetPath
7479
cd sass-spec/spec/libsass/Sáss-UŢF8/
7580
&$env:TargetPath ./input.scss 2>&1>$null
7681
if(-not($?)) {

docs/dev-ast-memory.md

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
# LibSass smart pointer implementation
2+
3+
LibSass uses smart pointers very similar to `shared_ptr` known
4+
by Boost or C++11. Implementation is a bit less modular since
5+
it was not needed. Various compile time debug options are
6+
available if you need to debug memory life-cycles.
7+
8+
9+
## Memory Classes
10+
11+
### SharedObj
12+
13+
Base class for the actual node implementations. This ensures
14+
that every object has a reference counter and other values.
15+
16+
```c++
17+
class AST_Node : public SharedObj { ... };
18+
```
19+
20+
### SharedPtr (base class for SharedImpl)
21+
22+
Base class that holds on to the pointer. The reference counter
23+
is stored inside the pointer object directly (`SharedObj`).
24+
25+
### SharedImpl (inherits from SharedPtr)
26+
27+
This is the main base class for objects you use in your code. It
28+
will make sure that the memory it points at will be deleted once
29+
all copies to the same object/memory go out of scope.
30+
31+
```c++
32+
Class* pointer = new Class(...);
33+
SharedImpl<Class> obj(pointer);
34+
```
35+
36+
To spare the developer of typing the templated class every time,
37+
we created typedefs for each available AST Node specialization.
38+
39+
```c++
40+
typedef SharedImpl<Number> Number_Obj;
41+
Number_Obj number = SASS_MEMORY_NEW(...);
42+
```
43+
44+
45+
## Memory life-cycles
46+
47+
### Pointer pickups
48+
49+
I often use the terminology of "pickup". This means the moment when
50+
a raw pointer not under any control is assigned to a reference counted
51+
object (`XYZ_Obj = XYZ_Ptr`). From that point on memory will be
52+
automatically released once the object goes out of scope (but only
53+
if the reference counter reaches zero). Main point beeing, you don't
54+
have to worry about memory management yourself.
55+
56+
### Object detach
57+
58+
Sometimes we can't return reference counted objects directly (see
59+
invalid covariant return types problems below). But we often still
60+
need to use reference objects inside a function to avoid leaks when
61+
something throws. For this you can use `detach`, which basically
62+
detaches the pointer memory from the reference counted object. So
63+
when the reference counted object goes out of scope, it will not
64+
free the attached memory. You are now again in charge of freeing
65+
the memory (just assign it to a reference counted object again).
66+
67+
68+
## Circular references
69+
70+
Reference counted memory implementations are prone to circular references.
71+
This can be addressed by using a multi generation garbage collector. But
72+
for our use-case that seems overkill. There is no way so far for users
73+
(sass code) to create circular references. Therefore we can code around
74+
this possible issue. But developers should be aware of this limitation.
75+
76+
There are AFAIR two places where circular references could happen. One is
77+
the `sources` member on every `Selector`. The other one can happen in the
78+
extend code (Node handling). The easy way to avoid this is to only assign
79+
complete object clones to these members. If you know the objects lifetime
80+
is longer than the reference you create, you can also just store the raw
81+
pointer. Once needed this could be solved with weak pointers.
82+
83+
84+
## Addressing the invalid covariant return types problems
85+
86+
If you are not familiar with the mentioned problem, you may want
87+
to read up on covariant return types and virtual functions, i.e.
88+
89+
- http://stackoverflow.com/questions/6924754/return-type-covariance-with-smart-pointers
90+
- http://stackoverflow.com/questions/196733/how-can-i-use-covariant-return-types-with-smart-pointers
91+
- http://stackoverflow.com/questions/2687790/how-to-accomplish-covariant-return-types-when-returning-a-shared-ptr
92+
93+
We hit this issue at least with the CRTP visitor pattern (eval, expand,
94+
listize and so forth). This means we cannot return reference counted
95+
objects directly. We are forced to return raw pointers or we would need
96+
to have a lot of explicit and expensive upcasts by callers/consumers.
97+
98+
### Simple functions that allocate new AST Nodes
99+
100+
In the parser step we often create new objects and can just return a
101+
unique pointer (meaning ownership clearly shifts back to the caller).
102+
The caller/consumer is responsible that the memory is freed.
103+
104+
```c++
105+
typedef Number* Number_Ptr;
106+
int parse_integer() {
107+
... // do the parsing
108+
return 42;
109+
}
110+
Number_Ptr parse_number() {
111+
Number_Ptr p_nr = SASS_MEMORY_NEW(...);
112+
p_nr->value(parse_integer());
113+
return p_nr;
114+
}
115+
Number_Obj nr = parse_number();
116+
```
117+
118+
The above would be the encouraged pattern for such simple cases.
119+
120+
### Allocate new AST Nodes in functions that can throw
121+
122+
There is a major caveat with the previous example, considering this
123+
more real-life implementation that throws an error. The throw may
124+
happen deep down in another function. Holding raw pointers that
125+
we need to free would leak in this case.
126+
127+
```c++
128+
int parse_integer() {
129+
... // do the parsing
130+
if (error) throw(error);
131+
return 42;
132+
}
133+
```
134+
135+
With this `parse_integer` function the previous example would leak memory.
136+
I guess it is pretty obvious, as the allocated memory will not be freed,
137+
as it was never assigned to a SharedObj value. Therefore the above code
138+
would better be written as:
139+
140+
```c++
141+
typedef Number* Number_Ptr;
142+
int parse_integer() {
143+
... // do the parsing
144+
if (error) throw(error);
145+
return 42;
146+
}
147+
// this leaks due to pointer return
148+
// should return Number_Obj instead
149+
// though not possible for virtuals!
150+
Number_Ptr parse_number() {
151+
Number_Obj nr = SASS_MEMORY_NEW(...);
152+
nr->value(parse_integer()); // throws
153+
return &nr; // Ptr from Obj
154+
}
155+
Number_Obj nr = parse_number();
156+
// will now be freed automatically
157+
```
158+
159+
The example above unfortunately will not work as is, since we return a
160+
`Number_Ptr` from that function. Therefore the object allocated inside
161+
the function is already gone when it is picked up again by the caller.
162+
The easy fix for the given simplified use case would be to change the
163+
return type of `parse_number` to `Number_Obj`. Indeed we do it exactly
164+
this way in the parser. But as stated above, this will not work for
165+
virtual functions due to invalid covariant return types!
166+
167+
### Return managed objects from virtual functions
168+
169+
The easy fix would be to just create a new copy on the heap and return
170+
that. But this seems like a very inelegant solution to this problem. I
171+
mean why can't we just tell the object to treat it like a newly allocated
172+
object? And indeed we can. I've added a `detach` method that will tell
173+
the object to survive deallocation until the next pickup. This means
174+
that it will leak if it is not picked up by consumer.
175+
176+
```c++
177+
typedef Number* Number_Ptr;
178+
int parse_integer() {
179+
... // do the parsing
180+
if (error) throw(error);
181+
return 42;
182+
}
183+
Number_Ptr parse_number() {
184+
Number_Obj nr = SASS_MEMORY_NEW(...);
185+
nr->value(parse_integer()); // throws
186+
return nr.detach();
187+
}
188+
Number_Obj nr = parse_number();
189+
// will now be freed automatically
190+
```
191+
192+
193+
## Compile time debug options
194+
195+
To enable memory debugging you need to define `DEBUG_SHARED_PTR`.
196+
This can i.e. be done in `include/sass/base.h`
197+
198+
```c++
199+
define DEBUG_SHARED_PTR
200+
```
201+
202+
This will print lost memory on exit to stderr. You can also use
203+
`setDbg(true)` on sepecific variables to emit reference counter
204+
increase, decrease and other events.
205+
206+
207+
## Why reinvent the wheel when there is `shared_ptr` from C++11
208+
209+
First, implementing a smart pointer class is not really that hard. It
210+
was indeed also a learning experience for myself. But there are more
211+
profound advantages:
212+
213+
- Better GCC 4.4 compatibility (which most code still has OOTB)
214+
- Not thread safe (give us some free performance on some compiler)
215+
- Beeing able to track memory allocations for debugging purposes
216+
- Adding additional features if needed (as seen in `detach`)
217+
- Optional: optimized weak pointer implementation possible
218+
219+
### Thread Safety
220+
221+
As said above, this is not thread safe currently. But we don't need
222+
this ATM anyway. And I guess we probably never will share AST Nodes
223+
across different threads.

include/sass/base.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
#ifndef SASS_BASE_H
22
#define SASS_BASE_H
33

4+
// #define DEBUG_SHARED_PTR
5+
46
#ifdef _MSC_VER
57
#pragma warning(disable : 4503)
68
#ifndef _SCL_SECURE_NO_WARNINGS

script/test-leaks.pl

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
#!/usr/bin/perl
2+
############################################################
3+
# this perl script is meant for developers only!
4+
# it will run all spec-tests (without verifying the
5+
# results) via valgrind to detect possible leaks.
6+
# expect that it takes 1h or more to finish!
7+
############################################################
8+
# Prerequisite install: `cpan Parallel::Runner`
9+
# You may also need to install `cpan File::Find`
10+
# You may also need to install `cpan IPC::Run3`
11+
############################################################
12+
# usage: `perl test-leaks.pl [threads]`
13+
# example: `time perl test-leaks.pl 4`
14+
############################################################
15+
# leaks will be reported in "mem-leaks.log"
16+
############################################################
17+
18+
use strict;
19+
use warnings;
20+
21+
############################################################
22+
# configurations (you may adjust)
23+
############################################################
24+
25+
# number of threads to use
26+
my $threads = $ARGV[0] || 8;
27+
28+
# the github repositories to checkout
29+
# if you need other branch, clone manually!
30+
my $sassc = "https://www.github.com/sass/sassc";
31+
my $specs = "https://www.github.com/sass/sass-spec";
32+
33+
############################################################
34+
# load modules
35+
############################################################
36+
37+
use IPC::Run3;
38+
use IO::Handle;
39+
use Fcntl qw(:flock);
40+
use File::Find::Rule;
41+
use Parallel::Runner;
42+
use List::Util qw(shuffle);
43+
44+
############################################################
45+
# check prerequisites
46+
############################################################
47+
48+
unless (-d "../sassc") {
49+
warn "sassc folder not found\n";
50+
warn "trying to checkout via git\n";
51+
system("git", "clone", $sassc, "../sassc");
52+
die "git command did not exit gracefully" if $?;
53+
}
54+
55+
unless (-d "../sass-spec") {
56+
warn "sass-spec folder not found\n";
57+
warn "trying to checkout via git\n";
58+
system("git", "clone", $specs, "../sass-spec");
59+
die "git command did not exit gracefully" if $?;
60+
}
61+
62+
unless (-f "../sassc/bin/sassc") {
63+
warn "sassc executable not found\n";
64+
warn "trying to compile via make\n";
65+
system("make", "-C", "../sassc", "-j", $threads);
66+
die "make command did not exit gracefully" if $?;
67+
}
68+
69+
############################################################
70+
# main runner code
71+
############################################################
72+
73+
my $root = "../sass-spec/spec";
74+
my @files = File::Find::Rule->file()
75+
->name('input.scss')->in($root);
76+
77+
open(my $leaks, ">", "mem-leaks.log");
78+
die "Cannot open log" unless $leaks;
79+
my $runner = Parallel::Runner->new($threads);
80+
die "Cannot start runner" unless $runner;
81+
82+
print "##########################\n";
83+
print "Testing $#files spec files\n";
84+
print "##########################\n";
85+
86+
foreach my $file (shuffle @files) {
87+
$runner->run(sub {
88+
$| = 1; select STDOUT;
89+
my $cmd = sprintf('../sassc/bin/sassc %s', $file);
90+
my $check = sprintf('valgrind --leak-check=yes %s', $cmd);
91+
run3($check, undef, \ my $out, \ my $err);
92+
if ($err =~ m/in use at exit: 0 bytes in 0 blocks/) {
93+
print "."; # print success indicator
94+
} else {
95+
print "F"; # print error indicator
96+
flock($leaks, LOCK_EX) or die "Cannot lock log";
97+
$leaks->printflush("#" x 80, "\n", $err, "\n");
98+
flock($leaks, LOCK_UN) or die "Cannot unlock log";
99+
}
100+
});
101+
}
102+
103+
$runner->finish;

0 commit comments

Comments
 (0)