Arc Forumnew | comments | leaders | submitlogin
2 points by akkartik 5050 days ago | link | parent

Here's my implementation of persisted.

  (mac fread(filename val)
    (let f (uniq)
      `(w/infile ,f ,filename
          (= ,val (unserialize:read ,f)))))

  (def fwrite(filename val)
    (w/outfile f filename
      (write serialize.val f)))

  (def fwritefile(filename val)
    (let tmpfile (+ filename ".tmp")
      (fwrite tmpfile val)
      (until file-exists.tmpfile)
      (mvfile tmpfile filename)))

  (mac load-snapshot(var initval)
    `(aif (file-exists ,(stringify var))
        (fread it ,var)
        (= ,var ,initval)))

  (mac save-snapshot(var)
    `(fwritefile ,(stringify var) ,var))

  (= persisted-vars* ())
  (mac persisted(var initval)
    `(do
       (load-snapshot ,var ,initval)
       (pushnew ',var persisted-vars*)))
I'm less certain about the part that actually does the autosaving; you'll want to adjust it for your situation. Here's what works for me:

  (def autosave-state()
    (while t
      (each var persisted-vars*
        (eval `(save-snapshot ,var)))
      (sleep 300)))
  (new-thread autosave-state)
This is less efficient that ptable, but ptable doesn't work for nested tables; you can't detect writes to inner tables.


4 points by fallintothis 5047 days ago | link

Sorry for the unsolicited code review (I don't mean to be malicious), but the implementation seems overcomplicated: wheel reinvention that's inconsistent with Arc's built-ins.

Why is fread a macro? You can easily set a variable explicitly when you want it. Moreover, the basic functionality is like Arc's readfile1, not read. It can simplify to a composition of two functions:

  (def freadfile1 (name)
    (unserialize (readfile1 name)))
If you wanted something more like readfile that unserializes each form, you can just amend Arc's definition:

  (def freadfile (name)
    (w/infile s name (drain (unserialize (read s)))))
I suppose there's (map unserialize (readfile name)), but that's an extra pass with more consing.

fwrite and fwritefile appear to duplicate functionality -- one just makes a temp file first. I assume you got the idea from Arc's writefile, except you added (until file-exists.tmpfile). Is there a reason for this? Did you have issues with trying to move the file before it was finished writing?

That point notwithstanding, they can still be rewritten closer to Arc's built-ins:

  (def fwrite (val (o output (stdout)))
    (write (serialize val) output))

  (def fwritefile (val name)
    (writefile (serialize val) name))
The f prefixing all these names confuses me. What is it supposed to stand for? "File"?

load-snapshot uses stringify, which I assume is supposed to be Arc's string (which works just fine for converting symbols to strings). It also has a variable capture issue since it uses aif. E.g., using your original definition with (= stringify string):

  arc> (load "macdebug.arc") ; see http://www.arclanguage.org/item?id=11806
  arc> (macsteps '(load-snapshot it init) 'show 'load-snapshot 'aif)
  Showing only: load-snapshot, aif

  Expression:

    (load-snapshot it init)

  Macro Expansion:

      (load-snapshot it init)

  ==> (aif (file-exists "it")
           (fread it it)
           (= it init))

  Expression:

    (aif (file-exists "it")
         (fread it it)
         (= it init))

  Macro Expansion:

      (aif (file-exists "it")
           (fread it it)
           (= it init))

  ==> (let it (file-exists "it")
        (if it (fread it it) (= it init)))

  Expression:

    (let it (file-exists "it")
      (if it (fread it it) (= it init)))

  nil
Really, what's happening between load-snapshot and save-snapshot is encapsulated by Arc's fromdisk and todisk (added benefit: no variable capture).

  (mac load-snapshot (var init)
    `(fromdisk ,var ,(string var) ,init freadfile1 fwritefile))

  (mac save-snapshot (var)
    `(todisk ,var))
persisted can also use do1 to be more like fromdisk. Instead of returning the list of persisted variables, it will return the actual variable's value. I don't know if your behavior was intentional, but it's an easy change.

  (= persisted-vars* nil)

  (mac persisted (var init)
    `(do1 (load-snapshot ,var ,init)
          (pushnew ',var persisted-vars*)))
Finally, why not make autosave-state thread itself, instead of having to remember to call new-thread?

  (def autosave-state ()
    (thread
      (while t
        (each var persisted-vars*
          (eval `(save-snapshot ,var)))
        (sleep 300))))
Altogether, that's:

  (def freadfile1 (name)
    (unserialize (readfile1 name)))

  (def fwritefile (val name)
    (writefile (serialize val) name))

  (mac load-snapshot (var init)
    `(fromdisk ,var ,(string var) ,init freadfile1 fwritefile))

  (mac save-snapshot (var)
    `(todisk ,var))

  (= persisted-vars* nil)

  (mac persisted (var init)
    `(do (load-snapshot ,var ,init)
         (pushnew ',var persisted-vars*)))

  (def autosave-state ()
    (thread
      (while t
        (each var persisted-vars*
          (eval `(save-snapshot ,var)))
        (sleep 300))))
This is more consistent with Arc's counterparts and makes good use of existing functionality. Indeed, load-snapshot is Arc's diskvar, except we lace it with serialize & unserialize to handle different structures generically, which is useful.

It could be shorter if we accept freadfile1, fwritefile, load-snapshot, and save-snapshot as implementation details of persisted:

  (= persisted-vars* nil)

  (mac persisted (var init)
    (w/uniq (val file)
      `(do1 (fromdisk ,var
                      ,(string var)
                      ,init
                      unserialize:readfile1
                      (fn (,val ,file) (writefile (serialize ,val) ,file)))
            (pushnew ',var persisted-vars*))))

  (def autosave-state ()
    (thread
      (while t
        (each var persisted-vars*
          (eval `(todisk ,var)))
        (sleep 300))))
Anyway, I think the autosaving and un/serialize generic combo is pretty cool. It lets you extend the functionality more than Arc's existing fromdisk stuff, since you just make a new generic function to handle your data; then you get to keep using persisted.

-----

2 points by akkartik 5047 days ago | link

Thanks a lot. My implementation is an accretion since I first started playing with arc. I no longer remember what the 'f' prefix stands for :) Perhaps it should be gread and gwrite for generic or generalized..

I'm going to spend a while going over the methods you pointed out - I still wasn't familiar with readfile1 or fromdisk. Thanks. Code review is always appreciated.

Update: I'm reminded of an earlier bit of code review from you that was super useful as well: http://arclanguage.org/item?id=10700

Also, folks may find it interesting to compare your solution and mine with a version from several months ago: http://arclanguage.org/item?id=10699

Update 2: Ah, fromdisk isn't in the arclanguage.org documentation! That helps me understand where my efforts have been lacking.

-----

1 point by akkartik 5047 days ago | link

"you added (until file-exists.tmpfile). Is there a reason for this? Did you have issues with trying to move the file before it was finished writing?"

Just looked through my changelogs. The answer is yes, I seem to have run into this issue at some point. writing is a disk-level creature where mv is a file-system-level beast. That seems to cause occasional issues.

-----