Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Concurrent Translations #1037

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Concurrent Translations #1037

wants to merge 3 commits into from

Conversation

zaneenders
Copy link
Collaborator

@zaneenders zaneenders commented Nov 9, 2024 •
edited
Loading

This PR moves the translation functions "inside" of the server. Currently, all translations are done on the "main thread" which is also where the server manager is handling requests sent to the server. This creates a potential bottleneck slowing down responses to consumers of Herbie's web API like Odyssey.

@zaneenders zaneenders force-pushed the zane-server-translate branch from 714c479 to 37695b4 Compare November 15, 2024 16:46
@zaneenders zaneenders force-pushed the zane-server-no-place-mode branch from 6983993 to 8fceacb Compare November 15, 2024 19:06
@zaneenders zaneenders marked this pull request as ready for review November 15, 2024 19:11
@zaneenders zaneenders mentioned this pull request Nov 15, 2024
Closed
@zaneenders zaneenders changed the title Server Translations Concurrent Translations Nov 16, 2024
@zaneenders zaneenders force-pushed the zane-server-translate branch from d9455d0 to 93726b9 Compare November 16, 2024 09:51
@zaneenders zaneenders force-pushed the zane-server-no-place-mode branch from 8fceacb to 73be763 Compare November 16, 2024 18:40
@zaneenders
Copy link
Collaborator Author

Fixed rebase of zane-server-no-place-mode.

Base automatically changed from zane-server-no-place-mode to main November 18, 2024 15:48
pavpanchekha
pavpanchekha requested changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@pavpanchekha pavpanchekha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this design at all. As far as I can tell, a half-dozen places in the code all special-case on whether the job is a translation job or not, and the user of server.rkt also has to know what kind of job they're requesting. Why?

src/api/server.rkt
Comment on lines -42 to +45
(define verbose #f) ; Maybe change to log-level and use 'verbose?
(define verbose #t) ; Maybe change to log-level and use 'verbose?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change? Debugging gunk?

src/api/server.rkt

(struct translate-job (fpcore to-language) #:prefab)

(define (_create-job0 server-action-type associated-type)
Copy link
Contributor

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 have such an ugly name? Why not just create-job?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why does this function even exist? Its whole goal is just creating a server-action struct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also why is the user of server.rkt responsible for knowing which actions go to the thread and which ones are done immediately?

src/api/server.rkt
@@ -87,6 +101,7 @@
(log "Getting result for job: ~a.\n" job-id)
(manager-ask 'result job-id))

; Invlaid to ask for timeline of a 'translate job.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
; Invlaid to ask for timeline of a 'translate job.
; Invalid to ask for timeline of a 'translate job.

src/api/server.rkt Outdated
@@ -119,26 +146,99 @@

(define (manager-tell msg . args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is manager-tell special-casing start behavior? The user doesn't call manager-tell!

src/api/server.rkt Outdated
Comment on lines 134 to 235
(not (sync/timeout 0 manager-dead-event)))
(if manager
(not (sync/timeout 0 manager-dead-event))
#t))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotta merge with main, I think this part of the diff is all from no-place being merged.

@zaneenders zaneenders force-pushed the zane-server-translate branch from 1979a2f to e34c789 Compare November 21, 2024 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavpanchekha pavpanchekha pavpanchekha requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zaneenders @pavpanchekha