-
Notifications
You must be signed in to change notification settings - Fork 1
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
Lparallel overhaul #16
base: master
Are you sure you want to change the base?
Conversation
We now exclusively use Lparallel.
4a09b13
to
6b3c4bc
Compare
cd116be
to
a78c02a
Compare
Turns out that serapeum:count-cpus is really slow.
This avoids spamming `update-source' when the input is updated too fast (< input-delay).
a78c02a
to
78d3a0d
Compare
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.
It's all clear and good overall. I mainly have some convenience suggestions:
- Move the kernel cleanup logic into a
(defmethod (setf kernel) ((value null) prompter) ...)
. - Add a wrapper
kernel
onsource
.
prompter-source.lisp
Outdated
(calispel:? wait-channel)) | ||
(if (listp (constructor source)) | ||
(setf (slot-value source 'initial-suggestions) (constructor source) | ||
(slot-value source 'initial-suggestions) (ensure-suggestions-list source (initial-suggestions source)) |
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.
Maybe a setf-method for initial-suggestions
instead?
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.
Oops, this code was poorly ported :) I'll fix it.
That said, how could a setf-method help here?
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.
Adding an ensure-suggestions-list
, for one.
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.
Actually the point of not having a writer is to hint that it should not be modified.
It's only ever set in this function.
Note that there's no |
|
So an attribute may not only be (KEY VALUE) but also (KEY VALUE EXTRA-1 ... EXTRA-N).
Attribute-value API is somewhat fixed:
This is sufficient for the Lparallel overhaul. There is more to fix in the attribute API, but I'll open a separate issue for that. |
Attribute API discussion: #21 |
Wait before merging, we need to make sure atlas-engineer/nyxt#2998 (comment) is happy with this new API. |
It's OK to merge from my side. Splendid work @Ambrevar! |
;; TODO: `slot-names' or `direct-slot-names'? | ||
#-ecl | ||
(mopu:slot-names class-specifier) | ||
#+ecl |
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.
Actually, why? Seems like mopu:slot-names
definition is exactly what you're reproducing here:
(defun class-slot-names (thing)
(let ((class (get-class thing)))
(if class
(mapcar 'mop:slot-definition-name
(mop:class-slots (finalize-class-if-necessary class)))
(progn
(warn "class for ~a not found)" thing)
nil))))
EDIT: highlightling.
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.
Trusting André's review here :)
update-notifier was not practical (non-broadcasting, plus accumulating). It also involved more set up on the client side.
Last commit (cdef02f) makes a radical change: it replaces The rationale is that @jmercouris @aadcg @aartaka Thoughts? |
To do:
|
The proper name would be |
Back to this: this has become a behemoth change and it's still not working... In any case, I'll split this pull request in two parts:
|
@Ambrevar sounds like an excellent plan! |
Fixes #2.
This switches the whole thread management / concurrency logic from Calispel to Lparallel.
To do:
Should work once the following issue "attempt to submit task to dead kernel" is fixed.
Attempt to submit task to dead kernel.
error when running multiple times.wait-on-prompt-buffer
to API.No need, it's now enough to force
result-channel
.lpara:task-handler-bind
.