-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Server no place mode #995
base: main
Are you sure you want to change the base?
Server no place mode #995
Conversation
6c2583f
to
12f2b01
Compare
This PR does make the server more complex as it can now operate in two modes. But my goal with this PR and future PRs is to reduce the split code paths as well as introduce duplicate logic in Herbie. |
(place-channel-get a)] | ||
[else | ||
(define result (hash-ref single-threaded-cache job-id #f)) | ||
(if (false? result) (reverse (unbox (*timeline*))) result)])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if result result (reverse ...))
or just (and result (reverse ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also is this correct? If there is a result it looks like it'll save it.
Also wait why goes into single-threaded-cache
? In fact, how can we ever call this method while the timeline is running? Why not just always return #f
?
@@ -79,56 +79,98 @@ | |||
(rename-file-or-directory tmp-file data-file #t) | |||
(copy-file (web-resource "report.html") html-file #t)) | |||
|
|||
(define single-threaded-cache (make-hash)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this have to be a separate cache from whatever the manager uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I thought it had to be inside the manager for isolation reasons but main
seems to work if I make completed-work
a global.
(define job-thread | ||
(thread (λ () | ||
(let loop ([seed #f]) | ||
(match (thread-receive) | ||
[(list work job-id semaphore) (single-thread-herbie work job-id semaphore)]) | ||
(loop seed))))) | ||
(define sema (make-semaphore)) | ||
(set! job-running #t) | ||
(thread-send job-thread (list command job-id sema)) | ||
(semaphore-wait sema) | ||
(set! job-running #f)]) ;; Block for job to finish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put this in a thread? Won't this thread keep running forever? Don't we only send it one job? Why have a thread at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if there was no thread you wouldn't need a semaphor. And if you did that you wouldn't need the single-thread-herbie
function which does very little on its own.
(log "Job ~a, Qed up for program: ~a\n" job-id (test-name (herbie-command-test command))) | ||
(place-channel-put manager (list 'start manager command job-id))] | ||
[else | ||
(log "Waiting for job ~a to finish.\n" job-id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong long message?
(define finished-result (place-channel-get a)) | ||
(log "Done waiting for: ~a\n" job-id) | ||
finished-result] | ||
[else (hash-ref single-threaded-cache job-id #f)])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this ever returns #f
in multi-threaded-mode? Maybe just make it crash in single-threaded mode if you wait for a job you didn't start. (Also maybe in multi-threaded mode?)
(set! manager-dead-event (place-dead-evt r)) | ||
(set! manager r)) | ||
(cond | ||
[(> job-cap 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think single-threaded mode should be separate from the concept of 1 thread. Maybe just make it a separate method or a global parameter or something?
(define threads #f) | ||
(define threads 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I think #f
for no threads and 1
for one thread should be separate (because even in "1 thread" mode you in fact spawn a number of places, even though only one of them is a worker thread.)
This PR adds a "single thread" mode which the goal of is to have a mode that does not use places. Places can cause difficult debugging of lower-level core Herbie code so having a mode that does not add this indirection is a must before being able to use the server for other parts of Herbie like reports which generate the nightly.
In this mode, jobs are run when
start-job
is called and not whenwait-for-job
is called in order to support the demo. However, the demo doesn't stream the timeline and blocks until the job is completed.