-
Notifications
You must be signed in to change notification settings - Fork 111
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
Use c3 linearization in the MOP #1033
Conversation
✅ Deploy Preview for elastic-ritchie-8f47f9 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Builds upon the |
dc2d52e
to
b81f43a
Compare
54b9b04
to
08e591d
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.
Generally looks good, but can you please avoid the internal splicing defs as it makes the code slower?
Also, can you optimize a tad the intermediate garbage lists?
I suspect the code is about twice as slow as it could be.
I will add the slicing let form for v0.19
so that we don't have this problem in the future.
PTAL |
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.
looks mostly good, some cosmetic comments.
96cde9f
to
65e6169
Compare
PTAL |
PTAL |
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.
Some more comments for using the low level fx ops, we know they are fixnum and we don't need the type checks there.
Other than that, I think we should rename the plist/alist thing to properties
.
But let's move this forward, the rename can happen in the next step to alleviate some pain.
PTAL |
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.
there are a few unrelated things, maybe we should pick them up for separate prs.
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.
can we resolve the open threads?
Use c3 linearization for the class precedence list. Use the class precedence list for the slot index allocation. Test and document it all. Define defmutable in prelude. Add a (dump-backtrace?) parameter to control backtrace printing. Support build-manifest in the runtime. Distinguish cases of "Bad syntax". Regenerate the runtime bootstrap. std/misc/rtd: add type=? Integrate feedback from review. Fix c3-test for destructive operations More review feedback Checkpointing progress with MOP refactoring
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 ok, let's merge it and follow up.
I left a big pile of comments for the follow up(s).
doc/reference/dev/bootstrap.md
Outdated
- The solution is to create a *new* API with *new* names that | ||
must absolutely not clash with the old names. | ||
Add a suffix or prefix such as `*`, `/2` or `%`, or take the opportunity | ||
to give functions better, longer and/or more systematic names. |
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.
let's drop the "longer" :)
doc/reference/std/errors.md
Outdated
|
||
This parameter is notably used by the `display-exception` method for | ||
the `Error` type in the runtime, and by `with-exception-stack-trace` | ||
(see above) that is also used in the default exceptio handler installed |
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.
exceptio*n
@@ -27,6 +27,7 @@ | |||
"gerbil/runtime/system" | |||
"gerbil/runtime/loader" | |||
"gerbil/runtime/control" | |||
"gerbil/runtime/c3" |
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 we should collect those in a separate text file for inclusion, I hate myself for having 3 different versions of this list; can happen in a subsequent pr.
src/gerbil/prelude/core.ss
Outdated
;; c3 linearization | ||
c3-linearize not-null? remove-nulls append-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.
we'll need to groups those better, c3-linearize goes with the mp, the other two are general utilities.
src/gerbil/prelude/core.ss
Outdated
(def (class-opt? key) | ||
(memq (stx-e key) '(slots: id: name: plist: constructor: unchecked:))) | ||
(def (class-opt? key) | ||
(memq (stx-e key) '(slots: id: name: alist: constructor: unchecked:))) |
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.
these should become properties:
, but we can do in follow up.
src/gerbil/runtime/mop.ss
Outdated
;; In exchange for which 3. it's somewhat simpler and slightly faster in | ||
;; the usual case that doesn't involve a call to next-method. |
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.
probably quite a bit more than slightly :)
src/gerbil/runtime/mop.ss
Outdated
;; Changing the protocol to access an explicit super argument would be | ||
;; semantically nicer and would enable linear complexity for next-method | ||
;; (in the number of classes, or even just in the number of applicable method, | ||
;; if resolved only once), but would be notably more complex and somewhat | ||
;; incompatible (or involve much cleverness for backward compatibility). |
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.
this comment is quite misleading i think, do we need it?
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.
Basically there is a tradeoff: either you have faster and simpler/leaner code for 99% of the cases, and let 1% have a quadratic which is usually small, or make verything slower/more bloated for that 1%....
f*ck the 1%! occupy LiSP! :)
(defrules defmutable () | ||
((_ var value) (begin (def var value) (set! var var) (void)))) | ||
|
||
(def gerbil-system-manifest |
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.
this ends up being completely unrelated and maybe we should pull it to a separate pr... oh well, probably too much work and pain.
Runtime improvements
MOP:
Also:
Regenerate bootstrap.