Re: [Sbcl-devel] [patch] Disabling some AREF bounds checks

[Christophe R. <cs...@ca...>]

Juho Snellman <js...@ik...> writes:

> The attached patch disables bounds checks in REPLACE, VECTOR-POP and
> VECTOR-PUSH-EXTEND. In all of the above, the check has already been
> made in some form before the AREF, so the access is safe [*].
>
> A few benchmark results (versus vanilla 0.8.8.23):
>
> fill-strings/adjustable  [     95.04]   0.65
> DEFLATE-FILE             [     10.61]   0.84
> CLOS/defclass            [      9.54]   0.94
> CLOS/defmethod           [     19.58]   0.96
> CLOS/instantiate         [     31.98]   0.96

Nice.  I've merged your patch into sbcl-0.8.8.30.  Thank you.

> [*] The access isn't necessarily safe in multi-threaded programs.  For
>     example, it's possible that while one thread is doing a REPLACE,
>     another decreases the size of the target vector => the first
>     thread writes out of bounds. Then again, I believe same problem
>     already exists, since the size of the vector could change after
>     AREF calls %CHECK-BOUNDS but before it does the actual access.

You're right, the same problem does already exist, at least as I read
the code.

>     Whether "it's already not thread-safe" is a good enough excuse to
>     make something even less thread-safe (especially just for a 
>     performance hack, even if a moderately useful one) is another
>     question. The answer is probably no, but I'm sending this to the
>     list since I'd been bragging about the fill-strings/adjustable
>     numbers on #lisp before thinking of the threading issues... :-)

I'm taking the "yes" answer, actually.  There are two forms of
threadsafety which we need to concern ourselves about.  The first is
about the reentrancy of our procedures or functional units -- use of
global state (such as specials holding scratch space) or local state
(closure variables) can be non-threadsafe, and this is probably the
first thing to guard against (not only for threadsafety reasons, but
also because of bugs which can manifest in non-threaded programs:
consider the bug -- at least partially fixed now -- involving a
predicate which called SORT being passed as a predicate to SORT...).
This kind of threadsafety should definitely be eradicated, even at
substantial performance cost; currently in some regions the
performance cost is effectively infinite, as we have a global lock
around some regions.

The second form of threadsafety is more interesting, but much harder
to deal with, and is the kind that is (slightly) worsened by your
patch; the problem can be stated as maintaining the integrity of the
lisp environment when two or more processes make destructive
modifications to the same underlying lisp object (including two arrays
displaced to the same third array).  It's not entirely clear what we
can do here; it might be interesting to do some experiments involving
adding a spinlock word to every object, and mediating all access
through this lock; perhaps providing a build option for this.  (Bear
in mind that when I say "interesting" I mean as an academic exercise;
I feel that the practical solution, unless I'm ignoring some
state-of-the-art work in Erlang or elsewhere, is to say "don't let two
threads mutate the same object").  CHANGE-CLASS, along with extensive
CLOS cacheing provides some amusing difficulties on the way, I
suspect.

Anyway, it seems unreasonable to penalise the overwhelmingly common
case, just for a slightly less common failure mode in a programming
style which my instinct says deserves to lose; given this, I don't see
a problem with your optimization.  Thank you again.

Cheers,

Christophe
-- 
http://www-jcsu.jesus.cam.ac.uk/~csr21/       +44 1223 510 299/+44 7729 383 757
(set-pprint-dispatch 'number (lambda (s o) (declare (special b)) (format s b)))
(defvar b "~&Just another Lisp hacker~%")    (pprint #36rJesusCollegeCambridge)