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.

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

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Lparallel overhaul #16

wants to merge 50 commits into from

Conversation

Ambrevar
Copy link
Member

@Ambrevar Ambrevar commented Apr 28, 2023

Fixes #2.

This switches the whole thread management / concurrency logic from Calispel to Lparallel.

  • It simplifies the code a lot (about 100 lines less!).
  • It also makes it way less prone to race conditions, and much easier to debug.
  • Also less risk of dangling threads.
  • Finally, it may work on ECL. Still need to be tested.

To do:

  • Test on ECL.
    Should work once the following issue "attempt to submit task to dead kernel" is fixed.
  • Make sure there is no Attempt to submit task to dead kernel. error when running multiple times.
  • Fix attribute-value API.
  • Add wait-on-prompt-buffer to API.
    No need, it's now enough to force result-channel.
  • Make sure errors can be lpara:task-handler-bind.
  • Make sure no threads are left hanging, especially after running the test suite.

@Ambrevar Ambrevar force-pushed the lparalle-overhaul branch 3 times, most recently from 4a09b13 to 6b3c4bc Compare May 26, 2023 10:21
@Ambrevar
Copy link
Member Author

@aartaka @aadcg This is ready for review.

The main point of discussion that's left is this attribute*-value* API. It's a bit clumsy, we need better names and possibly less convoluted functions.

Copy link
Contributor

@aartaka aartaka left a 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 on source.

prompter-source.lisp Show resolved Hide resolved
prompter-source.lisp Outdated Show resolved Hide resolved
prompter-source.lisp Outdated Show resolved Hide resolved
(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))
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

prompter-source.lisp Outdated Show resolved Hide resolved
prompter.lisp Show resolved Hide resolved
prompter.lisp Outdated Show resolved Hide resolved
prompter.lisp Outdated Show resolved Hide resolved
prompter.lisp Outdated Show resolved Hide resolved
prompter.lisp Show resolved Hide resolved
@aartaka
Copy link
Contributor

aartaka commented May 31, 2023

Note that there's no lpara:promise type, there's only lparallel.promise::%promise. So maybe pick a different type?

@Ambrevar
Copy link
Member Author

Ambrevar commented Jun 6, 2023

result-channel renamed to result and blocking accessor added.

@Ambrevar
Copy link
Member Author

Ambrevar commented Jun 8, 2023

Attribute-value API is somewhat fixed:

  • We now export attribute-options and attributes-options methods so that the call site does not need to parse lists;
  • attribute-default calls to attribute-value, so that the future is properly forced.
  • attribute-value return "" when not ready, which is consistent with the previous behaviour.
  • Future's computation starts when object-properties is called.

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.

@Ambrevar Ambrevar mentioned this pull request Jun 8, 2023
@Ambrevar
Copy link
Member Author

Ambrevar commented Jun 8, 2023

Attribute API discussion: #21

@Ambrevar Ambrevar changed the title WIP: Lparallel overhaul Lparallel overhaul Jun 8, 2023
@Ambrevar
Copy link
Member Author

Ambrevar commented Jun 8, 2023

OK to merge? @aartaka @aadcg

@Ambrevar
Copy link
Member Author

Ambrevar commented Jun 8, 2023

Wait before merging, we need to make sure atlas-engineer/nyxt#2998 (comment) is happy with this new API.

@aadcg
Copy link
Member

aadcg commented Jun 8, 2023

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
Copy link
Contributor

@aartaka aartaka Jun 9, 2023

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.

Copy link
Contributor

@aartaka aartaka left a 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.
@Ambrevar
Copy link
Member Author

Last commit (cdef02f) makes a radical change: it replaces update-notifier by update-hook (from Nhooks!).

The rationale is that update-notifier was clumsy to use (and indeed never used in Nyxt). With update-hook, it should become trivial to have Nyxt prompt-render-suggestions on each update, thus removing the need for a complex setup.

@jmercouris @aadcg @aartaka Thoughts?

@Ambrevar
Copy link
Member Author

To do:

  • Maybe define a new update-hook type where the handlers only accept a source argument, instead of using hook-any.

@aartaka
Copy link
Contributor

aartaka commented Jun 28, 2023

  • Maybe define a new update-hook type where the handlers only accept a source argument, instead of using hook-any.

The proper name would be hook-source, but yes, that won't hurt!

@Ambrevar
Copy link
Member Author

Back to this: this has become a behemoth change and it's still not working...
Input buffering is not behaving as expected: either it's a mistake of mine, or there is something wrong with Lparallel.

In any case, I'll split this pull request in two parts:

  • First part includes all the bug fixes, the switch to Lparallel, etc. EXCEPT input buffering. This we can probably merge quickly.
  • Second part will be about input buffering, but it needs much more work.

@aadcg
Copy link
Member

aadcg commented Dec 13, 2023

@Ambrevar sounds like an excellent plan!

@Ambrevar Ambrevar mentioned this pull request Dec 18, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Switch to Lparallel and use conditions to cancel threads
3 participants