Re: [epsilon-devel] [Patch] Integrating Jitter as a sub-package in Poke: first iteration

[Jose E. Marchesi]

Hi Luca!
    
    Last night I committed sub-package mode in Jitter, and rebased to
    master:
      
http://ageinghacker.net/git/cgit.cgi/jitter/commit/?id=d2eb00486f118a60ce1677360237127d982cba00
    
    As promised I ported Poke to the latest Jitter, switching from Jitter as
    a dependency to Jitter as a sub-package.  You can find the first
    iteration of the changes attached here.

Awesome, thanks, you are the best :))
Please see my comments below.
    
    Please tell me what you think, José.  Before going further and cleaning
    up the changes with a ChangeLog entry, let me add a few remarks:
    
    a) I have changed "program" to "routine" in the context of the Poke VM,
       whenever I came across to that in comments.  It could probably be
       done in a more systematic way; tell me if I should go ahead.
       I have updated HACKING to introduce the idea of non-executable and
       executable routine, which has become important now.
    
    b) In this context, I had to make a choice in several data structures
       of yours which used to hold "program" pointers from an older Jitter.
       The old programs have turned into "routines", sometimes non-executable,
       sometimes executable.
       Non-executable routines are used at code generation time; from a
       non-executable routine you can obtain an executable routine (which
       can be no longer edited, but is in an efficient dispatch-dependent
       form suitable for, well, execution).
       Each non-executable routine has a field pointing to its executable
       counterpart, and vice-versa, so apart from efficiency, you could
       have both but keep a pointer to either one, in every context.
       Non-executable and executable routine can be destroyed independently,
       and the destruction functions provided by Jitter update the pointer
       fields as appropriate to avoid dangling references: for example if
       you destroy an executable routine when its non-executable version
       still exists, the pointer from non-executable to executable becomes
       NULL.
       You make an executable routine from a non-executable routine by using
       (with your VM prefix)
         pvm_make_executable_routine
       , which is the new name of
         pvm_specialize_program
       . The important API difference is that now this function has a
       result.
       Now, which kind of routine shall I use in your own data structures?
       I decided in favor of executable routines almost in every case, to
       avoid a pointer dereference before execution, which may or may not
       matter for performance.  The one exception is the Poke assembler.
       I think I have been consistent with variable and field names.

Hm, so now you have routines and "executable routines" as separated
concepts.  You can go from the first to the second, and there is a link
back:

        routine  >>> executable routine
            ^               |
            |               |
            +---------------+

Something doesn't feel right here... isn't being executable (and
therefore not editable) a property of a routine?  If so, why not
exposing it as a property, instead of adding a new separated (but
coupled, the weird "link back") abstraction?

This way you don't need to expose two different types, and I don't need
to mess with that weird one-directional link from "executable routine"
to "routine" like in:

-        pvm_destroy_program (argv[i].val.prog);
+        {
+          pvm_routine rout = argv[i].val.erout->routine;
+          if (rout != NULL)
+            pvm_destroy_routine (rout);
+          pvm_destroy_executable_routine (argv[i].val.erout);
+        }

Having both pvm_destroy_routine and pvm_destroy_executable_routine,
why??  I want to destroy a routine, period.  I don't care whether the
routine is executable, or if it is editable, or if it contains debug
info, or if it is relocatable, or ...  I just expect pvm_destroy_routine
to do The Right Thing (TM), regardless of the characteristics/properties
of the routine.

The same applies to the other calls, and the split into two types.  And
that erout->routine... I really don't get it :)

Have you considered expanding on the current "modal" approach instead?
Consider this:

- Jitter routines are created non-executable/editable.  They can be
  edited.
- You can make them executable, pvm_make_routine_executable.
- You can make them non-executable/editable with a suitable function,
  unless the routine has been stripped.  pvm_make_routine_editable.
- You can strip them with a suitable function. pvm_make_strip_routine.

That would work nicely, I think, and allows you to make routines as
complicated as you want, while keeping a natural and nice interface.

    c) I have not carefully checked the build dependencies ensuring jitter
       (the code generator) is built.  An explicit
         $(MAKE) -C jitter/
       may be needed somewhere.

Ok.
    
    d) I have not touched the build system code selecting a Jitter dispatch;
       however this is no longer needed, and can be removed.

I'm not sure what you mean with "the build system code selecting a
Jitter dispatch".  Do you mean --with-debug?
    
    This is off-topic for the change set:
    
    e) I really think you should keep your C files generated by Jitter into
       the source directory.  This will make cross-compilation easy.

They are, in the distributed tarball.  Automake does that with sources
defined as BUILT.

    f) I saw that you copied autoconf/jitter.a4 from the Jitter distribution
       into acinclude.m4 .  I updated your copy with the new version, however
       I think you should switch to using AC_CONFIG_MACRO_DIRS and distribute
       unrelated Autoconf macro sets in different files, as recommended by
       the Automake manual (§"Handling Local Macros").  This will make
       integration easier if you include other Autoconf macro files, or add
       your own.

Yeah.  I could also acinclude the jitter.m4 file there.
    
    In any case Poke's test suite passes with no failures.  I normally use a
    separate build directory.
    
    Incidentally, Jitter's test suite will run as a side effect of running
    Poke's test suite.  I find this slightly annoying as Jitter's test suite
    takes some time even with only one dispatch enabled, and I think I will
    prevent this behavior by default.  This is a forthcoming change in
    Jitter, which should not require any modification on your side.

Yes, I would appreciate if you prevent that happening by default.
    
    Is all of this okay?

Regarding the subpackage stuff, I simply LOVE IT :)

Regarding the routine vs. executable routine split into two types,
instead of considering the later a specialized version of the former, I
really would like to understand the rationale behind it.