Skip to content

Lparallel overhaul - #16

Open
Ambrevar wants to merge 50 commits into
masterfrom
lparalle-overhaul
Open

Lparallel overhaul#16
Ambrevar wants to merge 50 commits into
masterfrom
lparalle-overhaul

Conversation

@Ambrevar

@Ambrevar Ambrevar commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

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
Ambrevar force-pushed the lparalle-overhaul branch from cd116be to a78c02a Compare May 31, 2023 07:46
@Ambrevar
Ambrevar force-pushed the lparalle-overhaul branch from a78c02a to 78d3a0d Compare May 31, 2023 07:54
@Ambrevar

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

@aartaka aartaka left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread prompter-source.lisp
Comment thread prompter-source.lisp Outdated
Comment thread prompter-source.lisp Outdated
Comment thread 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))

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

Comment thread prompter-source.lisp Outdated
Comment thread prompter.lisp
Comment thread prompter.lisp Outdated
Comment thread prompter.lisp Outdated
Comment thread prompter.lisp Outdated
Comment thread prompter.lisp
@aartaka

aartaka commented May 31, 2023

Copy link
Copy Markdown
Contributor

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

@Ambrevar

Ambrevar commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

result-channel renamed to result and blocking accessor added.

@Ambrevar

Ambrevar commented Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

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

Ambrevar commented Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

Attribute API discussion: #21

@Ambrevar Ambrevar changed the title WIP: Lparallel overhaul Lparallel overhaul Jun 8, 2023
@Ambrevar

Ambrevar commented Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

OK to merge? @aartaka @aadcg

@Ambrevar

Ambrevar commented Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

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

@aadcg

aadcg commented Jun 8, 2023

Copy link
Copy Markdown
Member

It's OK to merge from my side. Splendid work @Ambrevar!

Comment thread package.lisp
;; TODO: `slot-names' or `direct-slot-names'?
#-ecl
(mopu:slot-names class-specifier)
#+ecl

@aartaka aartaka Jun 9, 2023

Copy link
Copy Markdown
Contributor

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.

@aartaka aartaka left a comment

Copy link
Copy Markdown
Contributor

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 :)

Ambrevar added 2 commits June 26, 2023 16:14
update-notifier was not practical (non-broadcasting, plus accumulating).
It also involved more set up on the client side.
@Ambrevar

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

To do:

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

@aartaka

aartaka commented Jun 28, 2023

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

aadcg commented Dec 13, 2023

Copy link
Copy Markdown
Member

@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