]> xenbits.xensource.com Git - xen.git/commitdiff
tools/oxenstored: Replace hand rolled GC with weak GC references
authorEdwin Török <edvin.torok@citrix.com>
Fri, 8 Jan 2021 11:57:37 +0000 (11:57 +0000)
committerAndrew Cooper <andrew.cooper3@citrix.com>
Fri, 22 Jan 2021 18:01:34 +0000 (18:01 +0000)
The code here is attempting to reduce memory usage by sharing common
substrings in the tree: it replaces strings with ints, and keeps a string->int
map that gets manually garbage collected using a hand-rolled mark and sweep
algorithm.

This is unnecessary: OCaml already has a mark-and-sweep Garbage Collector
runtime, and sharing of common strings in tree nodes can be achieved through
Weak references: if the string hasn't been seen yet it gets added to the Weak
reference table, and if it has we use the entry from the table instead, thus
storing a string only once.  When the string is no longer referenced OCaml's
GC will drop it from the weak table: there is no need to manually do a
mark-and-sweep, or to tell OCaml when to drop it.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
tools/ocaml/xenstored/connection.ml
tools/ocaml/xenstored/history.ml
tools/ocaml/xenstored/store.ml
tools/ocaml/xenstored/symbol.ml
tools/ocaml/xenstored/symbol.mli
tools/ocaml/xenstored/xenstored.ml

index 684f7a3861758aeffa9a8a4422b81f6e99b67b1d..d09a0fa405ab91c4f1fbfd47bc2bd0178853b5d6 100644 (file)
@@ -334,9 +334,6 @@ let has_more_work con =
 
 let incr_ops con = con.stat_nb_ops <- con.stat_nb_ops + 1
 
-let mark_symbols con =
-       Hashtbl.iter (fun _ t -> Store.mark_symbols (Transaction.get_store t)) con.transactions
-
 let stats con =
        Hashtbl.length con.watches, con.stat_nb_ops
 
index 3899353da8359d2f4d8672acacd93257564970e6..ba5c9cb571ff2a5bdaab00832c2247b61e7287e7 100644 (file)
@@ -22,20 +22,6 @@ type history_record = {
 
 let history : history_record list ref = ref []
 
-(* Called from periodic_ops to ensure we don't discard symbols that are still needed. *)
-(* There is scope for optimisation here, since in consecutive commits one commit's `after`
- * is the same thing as the next commit's `before`, but not all commits in history are
- * consecutive. *)
-let mark_symbols () =
-       (* There are gaps where dom0's commits are missing. Otherwise we could assume that
-        * each element's `before` is the same thing as the next element's `after`
-        * since the next element is the previous commit *)
-       List.iter (fun hist_rec ->
-                       Store.mark_symbols hist_rec.before;
-                       Store.mark_symbols hist_rec.after;
-               )
-               !history
-
 (* Keep only enough commit-history to protect the running transactions that we are still tracking *)
 (* There is scope for optimisation here, replacing List.filter with something more efficient,
  * probably on a different list-like structure. *)
index a3be2e6bbe45c6af32b6a21d33109f351e05b0f2..9c226e4ef7ff256efd70da88ed9921721199905a 100644 (file)
@@ -46,18 +46,18 @@ let add_child node child =
 
 let exists node childname =
        let childname = Symbol.of_string childname in
-       List.exists (fun n -> n.name = childname) node.children
+       List.exists (fun n -> Symbol.equal n.name childname) node.children
 
 let find node childname =
        let childname = Symbol.of_string childname in
-       List.find (fun n -> n.name = childname) node.children
+       List.find (fun n -> Symbol.equal n.name childname) node.children
 
 let replace_child node child nchild =
        (* this is the on-steroid version of the filter one-replace one *)
        let rec replace_one_in_list l =
                match l with
                | []                               -> []
-               | h :: tl when h.name = child.name -> nchild :: tl
+               | h :: tl when Symbol.equal h.name child.name -> nchild :: tl
                | h :: tl                          -> h :: replace_one_in_list tl
                in
        { node with children = (replace_one_in_list node.children) }
@@ -67,7 +67,7 @@ let del_childname node childname =
        let rec delete_one_in_list l =
                match l with
                | []                        -> raise Not_found
-               | h :: tl when h.name = sym -> tl
+               | h :: tl when Symbol.equal h.name sym -> tl
                | h :: tl                   -> h :: delete_one_in_list tl
                in
        { node with children = (delete_one_in_list node.children) }
@@ -489,9 +489,6 @@ let copy store = {
        quota = Quota.copy store.quota;
 }
 
-let mark_symbols store =
-       Node.recurse (fun node -> Symbol.mark_as_used node.Node.name) store.root
-
 let incr_transaction_coalesce store =
        store.stat_transaction_coalesce <- store.stat_transaction_coalesce + 1
 let incr_transaction_abort store =
index 4420c6a4d7ba7cf27791fa1ead1684b6b24ced20..2b41d120f66ebb9a61b44787d4ea7866e49e3e64 100644 (file)
  * GNU Lesser General Public License for more details.
  *)
 
-type t = int
+module WeakTable = Weak.Make(struct
+    type t = string
+    let equal (x:string) (y:string) = (x = y)
+    let hash = Hashtbl.hash
+end)
 
-type 'a record = { data: 'a; mutable garbage: bool }
-let int_string_tbl : (int,string record) Hashtbl.t = Hashtbl.create 1024
-let string_int_tbl : (string,int) Hashtbl.t = Hashtbl.create 1024
+type t = string
 
-let created_counter = ref 0
-let used_counter = ref 0
+let tbl = WeakTable.create 1024
 
-let count = ref 0
-let rec fresh () =
-       if Hashtbl.mem int_string_tbl !count
-       then begin
-               incr count;
-               fresh ()
-       end else
-               !count
+let of_string s = WeakTable.merge tbl s
+let to_string s = s
 
-let new_record v = { data=v; garbage=false }
-
-let of_string name =
-       if Hashtbl.mem string_int_tbl name
-       then begin
-               incr used_counter;
-               Hashtbl.find string_int_tbl name
-       end else begin
-               let i = fresh () in
-               incr created_counter;
-               Hashtbl.add string_int_tbl name i;
-               Hashtbl.add int_string_tbl i (new_record name);
-               i
-       end
-
-let to_string i =
-       (Hashtbl.find int_string_tbl i).data
-
-let mark_all_as_unused () =
-       Hashtbl.iter (fun _ v -> v.garbage <- true) int_string_tbl
-
-let mark_as_used symb =
-       let record1 = Hashtbl.find int_string_tbl symb in
-               record1.garbage <- false
-
-let garbage () =
-       let records = Hashtbl.fold (fun symb record accu ->
-               if record.garbage then (symb, record.data) :: accu else accu
-       ) int_string_tbl [] in
-       let remove (int,string) =
-               Hashtbl.remove int_string_tbl int;
-               Hashtbl.remove string_int_tbl string
-       in
-       created_counter := 0;
-       used_counter := 0;
-       List.iter remove records
+let equal a b =
+  (* compare using physical equality, both members have to be part of the above weak table *)
+  a == b
 
 let stats () =
-       Hashtbl.length string_int_tbl
-
-let created () = !created_counter
-let used () = !used_counter
+  let len, entries, _, _, _, _ = WeakTable.stats tbl in
+  len, entries
index c3c9f6e2f8245babb9653e559c7bdc8a87dc8fb0..586ab5750767a91b57283243a18832fe2d44e0a0 100644 (file)
@@ -29,24 +29,11 @@ val of_string : string -> t
 val to_string : t -> string
 (** Convert a symbol into a string. *)
 
-(** {6 Garbage Collection} *)
-
-(** Symbols need to be regulary garbage collected. The following steps should be followed:
--     mark all the knowns symbols as unused (with [mark_all_as_unused]);
--     mark all the symbols really usefull as used (with [mark_as_used]); and
--     finally, call [garbage] *)
-
-val mark_all_as_unused : unit -> unit
-val mark_as_used : t -> unit
-val garbage : unit -> unit
+val equal: t -> t -> bool
+(** Compare two symbols for equality *)
 
 (** {6 Statistics } *)
 
-val stats : unit -> int
-(** Get the number of used symbols. *)
-
-val created : unit -> int
-(** Returns the number of symbols created since the last GC. *)
+val stats : unit -> int * int
+(** Get the table size and number of entries. *)
 
-val used : unit -> int
-(** Returns the number of existing symbols used since the last GC *)
index 30edf4de6a5a7926579b8019548530f2f1779926..d44ae673c42a1da702ecb14f7623d47e8e4d0205 100644 (file)
@@ -424,18 +424,6 @@ let _ =
 
        let periodic_ops now =
                debug "periodic_ops starting";
-               (* we garbage collect the string->int dictionary after a sizeable amount of operations,
-                * there's no need to be really fast even if we got loose
-                * objects since names are often reuse.
-                *)
-               if Symbol.created () > 1000 || Symbol.used () > 20000
-               then begin
-                       Symbol.mark_all_as_unused ();
-                       Store.mark_symbols store;
-                       Connections.iter cons Connection.mark_symbols;
-                       History.mark_symbols ();
-                       Symbol.garbage ()
-               end;
 
                (* scan all the xs rings as a safenet for ill-behaved clients *)
                if !ring_scan_interval >= 0 && now > (!last_scan_time +. float !ring_scan_interval) then
@@ -453,11 +441,11 @@ let _ =
                        let (lanon, lanon_ops, lanon_watchs,
                             ldom, ldom_ops, ldom_watchs) = Connections.stats cons in
                        let store_nodes, store_abort, store_coalesce = Store.stats store in
-                       let symtbl_len = Symbol.stats () in
+                       let symtbl_len, symtbl_entries = Symbol.stats () in
 
                        info "store stat: nodes(%d) t-abort(%d) t-coalesce(%d)"
                             store_nodes store_abort store_coalesce;
-                       info "sytbl stat: %d" symtbl_len;
+                       info "sytbl stat: length(%d) entries(%d)" symtbl_len symtbl_entries;
                        info "  con stat: anonymous(%d, %d o, %d w) domains(%d, %d o, %d w)"
                             lanon lanon_ops lanon_watchs ldom ldom_ops ldom_watchs;
                        info "  mem stat: minor(%.0f) promoted(%.0f) major(%.0f) heap(%d w, %d c) live(%d w, %d b) free(%d w, %d b)"