The value -1 is not of type (UNSIGNED-BYTE 64) when setting slot CL-ISAAC::RANDCNT of structure CL-ISAAC:ISAAC64-CTX #12
Comments
|
@fukamachi probably this issue can be interesting to you. |
|
After the some time problem happened again. Seems something break the state of the context. Is it possible that context returned by |
|
Here is the minimal example how to reproduce this issue: What is interesting is that it always takes 85 iterations before the fail. |
|
The problem is in the decrement here: (defun rand64 (ctx)
(let ((c (isaac64-ctx-randcnt ctx)))
;(declare (optimize (speed 3) (safety 0)))
(decf (isaac64-ctx-randcnt ctx))
;; Here we are checking
(if (zerop c)
(progn
(generate-next-isaac64-block ctx)
(setf (isaac64-ctx-randcnt ctx) 255)
(aref (isaac64-ctx-randrsl ctx) 255))
(aref (isaac64-ctx-randrsl ctx) (isaac64-ctx-randcnt ctx)))))We can make a decrement even when (isaac64-ctx-randcnt ctx) is zero. But we need this decrement only in the second branch of the (defun rand64 (ctx)
;;(declare (optimize (speed 3) (safety 0)))
(cond
((zerop (isaac64-ctx-randcnt ctx))
(generate-next-isaac64-block ctx)
(setf (isaac64-ctx-randcnt ctx) 255)
(aref (isaac64-ctx-randrsl ctx) 255))
(t
(aref (isaac64-ctx-randrsl ctx)
(decf (isaac64-ctx-randcnt ctx))))))and it works! 32bit version should be fixed the same way. |
|
If you are ok with this solution, I can make a pull request. |
|
@thephoeron what do you think? |
|
Here are two functions to demostrate the problem. It only happens for particular compilation policies. (defun raises-an-error ()
(declare (optimize (safety 1) (debug 1) (speed 1) (space 1)))
(let ((c (make-isaac-ctx)))
(decf (isaac-ctx-randcnt c))))
(defun work-but-incorrectly ()
(declare (optimize (safety 0) (debug 1) (speed 1) (space 1)))
(let ((c (make-isaac-ctx)))
(decf (isaac-ctx-randcnt c))))Results: ISAAC> (raises-an-error)
; Debugger entered on #<TYPE-ERROR expected-type: (UNSIGNED-BYTE 32) datum: -1>
[1] ISAAC>
; Evaluation aborted on #<TYPE-ERROR expected-type: (UNSIGNED-BYTE 32) datum: -1>
ISAAC> (work-but-incorrectly)
-1 (0 bits)Tested on SBCL 2.3.3. When |
…ot CL-ISAAC::RANDCNT of structure CL-ISAAC:ISAAC64-CTX". This error happened when you set optimization policy `SAFETY > 0` in SBCL. Closes issue thephoeron#12.
|
Ok, I've created a pull request: #13 Will use this fixed version at Ultralisp server for a while. |
|
@svetlyak40wt is the fixed version available on ultralisp? I have this problem using reblocks. |
|
No, it will become available when this PR will be merged. For a while I'm using my branch with the fix in the |
|
@thephoeron how are you? can you please accept pull request #13 by @svetlyak40wt ? I had posted the issue in lack before investigating further and realizing it's a cl-isaac issue. @svetlyak40wt with your pull request, would you mind doing a test with a few threads to see if it is thread safe? We may as well figure it out now before running into it in the wild |
|
I don't think my pull request changes something related to the thread safety. The only thing it does – it moves the call to Actually, I've already use this patch in Ultralisp.org and it has no any threadsafety issues. However this does not grant there are no threadsafety issues. Now I've looked through the code and seems it changes context structure in many places. All of these changes are not done in "transaction" so, I presume, usage of a isaac context should not be thread safe :(. |
|
yeah, I didn't mean your code, I meant the underlying original cl-isaac code. I'm using your fork as well for my project, thanks. I just thought we may as well just test for thread safety since we are already here... |
|
Well, this is easy. Just run the code like this: CL-USER> (let ((ctx (cl-isaac:make-isaac64-ctx))
(num-threads 2))
(flet ((make-randoms ()
(loop repeat 100000000
for value = (cl-isaac:rand64 ctx)
sum value)))
(time
(loop for i below num-threads
collect (sb-thread:make-thread #'make-randoms) into threads
finally (mapc #'sb-thread:join-thread
threads)))))Even with my fork it produces this error: However for 1 thread it works fine. |
|
@svetlyak40wt thanks for checking that. Now a design question: Do you think the lock should be set in this library? Or since the seed variable is being passed in, it should be set by a client of the library instead? |
|
I think lock should not be taken in the library because this will cause slowdown in the single-threaded applications which use cl-isaac. Instead a warning about thread-safety should be added to the documentation. |
|
Ok great. I will try to send an email to @thephoeron for accepting the PR. Since you already have a fork and the PR, can you add the following to the README.md in a commit? |
|
Done. |
|
Same problem here. Need to liberalize the slot type specifiers for CL-ISAAC:ISAAC64-CTX or its broken.. |
Here is the backtrace:
Lack uses Isaac for generating random IDs. And I use this utility function in some places inside Ultralisp. After the recent release it started to fail with the error.
cl-isaac was built at 2023-06-26 14:19:17 by Ultralisp.
Here is the content of isaac64-ctx:
After I've reinitialized context like this:
problem gone.
The text was updated successfully, but these errors were encountered: