From 0d267c6165c90c1aa0811bd27c20bf01a9d85f13 Mon Sep 17 00:00:00 2001 From: Lucian Wischik Date: Tue, 10 Dec 2019 22:46:22 -0800 Subject: [PATCH] Defered_decl global mutable state Summary: My goal is that, when we typecheck, we can count how many decls needed to be fetched. I want to count that both for bulk typechecking invoked by `serverTypeCheck.do_typechecking` where it already happens (it invokes `Typing_check_service.go_with_interrupt` > `process_files`, or `process_in_parallel` > `process_files`). And also in `Provider_utils.compute_tast_and_errors_unquarantined` which is used by serverless-IDE and by hh_server to answer several smaller queries here and there. Defered_decls uses a bunch of mutable global state which makes this tricky -- e.g. can we reliably be working with that mutable state within `process_files`, and then handle an interrupt that wants its own decl counter, and then resume? The answer is a fragile "yes we can" because of the invariant that each `process_file` call never itself gets interrupted, and `Deferred_decl.reset` gets called at the start of each `process_file`, and the state is never read after the end before being reset by the next `process_file`. But I don't want to be fragile. Hence this diff. First, it introduces the API `Deferred_decl.restore_state`. The idea is that anyone who wants to do some local work can call `Deferred_decl.reset` as normal. But now it returns the previous global-mutable-state. Thus, when you've finished your local work, you can call `Deferred_decl.restore_state` so that other work can continue fine. In this way we can locally enforce the non-interference, rather than relying on a fragile global invariant. Second, this diff moves `threshold`. Previously it was being fetched by `typing_classes_heap` out of the global mutable variable `GlobalNamingOptions.opt`, and was being fetched each time a decl needed to be fetched. But for my purposes I wanted the local typecheck to never have a threshold. Therefore I'm making it so it's a part of the Deferred-decl state. Reviewed By: 2BitSalute Differential Revision: D18879092 fbshipit-source-id: bdfeb0dcd90ae5bd16f2804528418f8501b21fe8 --- hphp/hack/src/typing/deferred_decl.ml | 55 ++++++++++++++++++++-------- hphp/hack/src/typing/typing_check_service.ml | 6 ++- hphp/hack/src/typing/typing_classes_heap.ml | 8 +--- hphp/hack/test/unit/server_tests.ml | 15 +++----- 4 files changed, 51 insertions(+), 33 deletions(-) diff --git a/hphp/hack/src/typing/deferred_decl.ml b/hphp/hack/src/typing/deferred_decl.ml index 72cb6f7f1ef..815022f6020 100644 --- a/hphp/hack/src/typing/deferred_decl.ml +++ b/hphp/hack/src/typing/deferred_decl.ml @@ -14,29 +14,52 @@ type deferment = Relative_path.t type deferments_t = Relative_path.Set.t -let (deferments : deferments_t ref) = ref Relative_path.Set.empty +type deferred_decl_state = { + (* if enabled is false, then 'should_defer' is a no-op *) + enabled: bool; + (* deferments is grown by 'add' *) + deferments: deferments_t; + (* counter is incremented by 'should_defer', if enabled *) + counter: int; + (* threshold is checked against counter by 'should_defer', if enabled *) + threshold_opt: int option; +} -let (counter : int ref) = ref 0 +let state : deferred_decl_state ref = + ref + { + enabled = true; + deferments = Relative_path.Set.empty; + counter = 0; + threshold_opt = None; + } -let (enabled : bool ref) = ref true - -let should_defer ~(d : Relative_path.t) ~(threshold_opt : int option) : unit = - if !enabled then ( - counter := !counter + 1; - match threshold_opt with - | Some threshold when threshold < !counter -> raise (Defer d) +let should_defer ~(d : Relative_path.t) : unit = + if !state.enabled then ( + state := { !state with counter = !state.counter + 1 }; + match !state.threshold_opt with + | Some threshold when threshold < !state.counter -> raise (Defer d) | _ -> () ) let add ~(d : deferment) : unit = - deferments := Relative_path.Set.add !deferments d + state := + { !state with deferments = Relative_path.Set.add !state.deferments d } + +let restore_state (new_state : deferred_decl_state) : unit = state := new_state -let reset ~enable = - deferments := Relative_path.Set.empty; - counter := 0; - enabled := enable +let reset ~(enable : bool) ~(threshold_opt : int option) : deferred_decl_state = + let old_state = !state in + state := + { + enabled = enable; + counter = 0; + deferments = Relative_path.Set.empty; + threshold_opt; + }; + old_state let get_deferments ~(f : deferment -> 'a) : 'a list = - Relative_path.Set.fold !deferments ~init:[] ~f:(fun d l -> f d :: l) + Relative_path.Set.fold !state.deferments ~init:[] ~f:(fun d l -> f d :: l) -let get_counter () : int = !counter +let get_counter () : int = !state.counter diff --git a/hphp/hack/src/typing/typing_check_service.ml b/hphp/hack/src/typing/typing_check_service.ml index df0c9d7ae46..87db118fae1 100644 --- a/hphp/hack/src/typing/typing_check_service.ml +++ b/hphp/hack/src/typing/typing_check_service.ml @@ -169,7 +169,11 @@ let process_file (opts : GlobalOptions.t) (errors : Errors.t) (file : check_file_computation) : Errors.t * file_computation list = - Deferred_decl.reset ~enable:(should_enable_deferring opts file); + let (_old_state : Deferred_decl.deferred_decl_state) = + Deferred_decl.reset + ~enable:(should_enable_deferring opts file) + ~threshold_opt:(GlobalOptions.tco_defer_class_declaration_threshold opts) + in let fn = file.path in let ast = Ast_provider.get_ast ~full:true fn in let (funs, classes, record_defs, typedefs, gconsts) = Nast.get_defs ast in diff --git a/hphp/hack/src/typing/typing_classes_heap.ml b/hphp/hack/src/typing/typing_classes_heap.ml index 5c92a744824..2a03919fef1 100644 --- a/hphp/hack/src/typing/typing_classes_heap.ml +++ b/hphp/hack/src/typing/typing_classes_heap.ml @@ -55,10 +55,6 @@ let make_lazy_class_type class_name sc = let shallow_decl_enabled () = TypecheckerOptions.shallow_class_decl (Global_naming_options.get ()) -let defer_threshold () = - TypecheckerOptions.defer_class_declaration_threshold - (Global_naming_options.get ()) - module Classes = struct module Cache = SharedMem.LocalCache @@ -95,9 +91,7 @@ module Classes = struct | None -> raise Exit | Some (file, Naming_table.TClass) -> - Deferred_decl.should_defer - ~d:file - ~threshold_opt:(defer_threshold ()); + Deferred_decl.should_defer ~d:file; let class_type = Errors.run_in_decl_mode file (fun () -> Decl.declare_class_in_file file class_name) diff --git a/hphp/hack/test/unit/server_tests.ml b/hphp/hack/test/unit/server_tests.ml index f433530425e..035ace45673 100644 --- a/hphp/hack/test/unit/server_tests.ml +++ b/hphp/hack/test/unit/server_tests.ml @@ -40,7 +40,7 @@ let ensure_count (count : int) : unit = "The number of deferred items should match the expected value" let test_deferred_decl_add () = - Deferred_decl.reset ~enable:true; + let _old_state = Deferred_decl.reset ~enable:true ~threshold_opt:None in ensure_count 0; Deferred_decl.add (Relative_path.create Relative_path.Dummy "foo"); @@ -52,24 +52,23 @@ let test_deferred_decl_add () = Deferred_decl.add (Relative_path.create Relative_path.Dummy "bar"); ensure_count 2; - Deferred_decl.reset ~enable:true; + let _old_state = Deferred_decl.reset ~enable:true ~threshold_opt:None in ensure_count 0; true let ensure_threshold ~(threshold : int) ~(limit : int) ~(expected : int) : unit = - Deferred_decl.reset ~enable:true; + let _old_state = + Deferred_decl.reset ~enable:true ~threshold_opt:(Some threshold) + in ensure_count 0; let deferred_count = ref 0 in for i = 1 to limit do let path = Printf.sprintf "foo-%d" i in let relative_path = Relative_path.create Relative_path.Dummy path in - try - Deferred_decl.should_defer - ~d:relative_path - ~threshold_opt:(Some threshold) + try Deferred_decl.should_defer ~d:relative_path with Deferred_decl.Defer d -> Asserter.Bool_asserter.assert_equals (i >= threshold) @@ -124,8 +123,6 @@ let bar_contents = for files that have undeclared dependencies, UNLESS we've already deferred those files a certain number of times. *) let test_process_file_deferring () = - Deferred_decl.reset ~enable:true; - (* Set up a simple fake repo *) Disk.mkdir_p "/fake/root/"; Relative_path.set_path_prefix Relative_path.Root (Path.make "/fake/root/"); -- 2.11.4.GIT