Skip to content

Commit

Permalink
Back out fine-grained cache for FunctionDefinitionEnvironment
Browse files Browse the repository at this point in the history 
Summary:
This commit backs out a perf optimization I landed as part of trying to
unblock Pysa for the UGE cleanup where I made UnannotatedGlobalEnvironment
a "normal" Pyre environment instead of a complex set of fine-grained tables.

Because we saw Pysa regressions (which it turns out were the result of a
single slow-to-analyze module top-level that Maxime found and disabled), I
first split off `FunctionDefinitionEnvironment` and then made it so that
when dependency tracking is off (in `pyre check` and `pyre analyze`, for
example) we use a fine-grained table instead of the normal module-sope
table.

That second perf optimization, the fine-grained table, doesn't seem to actually
do much. It did help a little bit when we had that super slow-to-analyze module
top level enabled, but on trunk today perf results suggest that Pysa is actually
significantly faster *without* this extra complexity (the CallGraph computation,
which was the bottleneck before, is roughly unchanged but taint analysis
appears to run quite a bit faster)

Note that the `pyre check` vs `pyre incremental` delta that forced us to
revert D59770261 would have been impossible if the code had been in this state,
it was the fine-grained vs module-level table that caused a cold-start `incremental`
run to behave differently from `check`.

Note:

I am seeing evidence that some incremental problems still exist; getting this
diff in is helpful because it simplifies the logic that I need to debug here.

Reviewed By: grievejia

Differential Revision: D59965208

fbshipit-source-id: 62c524cda55d52d5c9c0b4019052d06e7c92dde5
  • Loading branch information
@stroxler @facebook-github-bot
stroxler authored and facebook-github-bot committed Jul 20, 2024
1 parent d34444e commit e8df7ec
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 109 deletions.
115 changes: 6 additions & 109 deletions source/analysis/functionDefinitionEnvironment.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@ module OutgoingDataComputation = struct
List.find_map ~f ancestors_descending


let define_names_of_qualifier function_definitions_of_qualifier qualifier =
function_definitions_of_qualifier qualifier
>>| Reference.Map.Tree.keys
|> Option.value ~default:[]


let function_definition function_definitions_of_qualifier function_name =
let load_function_definition_if_in_module qualifier =
function_definitions_of_qualifier qualifier
Expand Down Expand Up @@ -98,121 +92,24 @@ end)

include FunctionDefinitionsTable

module FineGrainedUntrackedLogic = struct
module SingleFunctionDefinitionValue = struct
type t = FunctionDefinition.t option [@@deriving equal]

let prefix = Hack_parallel.Std.Prefix.make ()

let description = "SingleFunctionDefinition"
end

module FunctionDefinitionNamesValue = struct
type t = Reference.t list [@@deriving equal]

let prefix = Hack_parallel.Std.Prefix.make ()

let description = "FunctionDefinitionNames"
end

module SingleFunctionDefinition = struct
include Memory.WithCache.Make (SharedMemoryKeys.ReferenceKey) (SingleFunctionDefinitionValue)
end

module FunctionDefinitionNames = struct
include Memory.WithCache.Make (SharedMemoryKeys.ReferenceKey) (FunctionDefinitionNamesValue)
end

let write_aside_single_function_definition ~key ~data =
SingleFunctionDefinition.write_around key (Some data)


let produce_value read_only qualifier =
let upstream_read_only = ReadOnly.upstream_environment read_only in
FunctionDefinitionsTable.In.produce_value ~dependency:None upstream_read_only qualifier


let produce_and_write_aside read_only ?(force_load = true) qualifier =
if force_load || FunctionDefinitionNames.get qualifier |> Option.is_none then
match produce_value read_only qualifier with
| None -> FunctionDefinitionNames.write_around qualifier []
| Some function_definitions ->
FunctionDefinitionNames.write_around
qualifier
(Reference.Map.Tree.keys function_definitions);
Reference.Map.Tree.iteri ~f:write_aside_single_function_definition function_definitions;
();
()


let produce_and_write_aside_function_definitions_all_parents read_only function_name =
Reference.all_parents function_name
|> List.iter ~f:(produce_and_write_aside ~force_load:false read_only)


let define_names_of_qualifier read_only ?dependency:_ qualifier =
match FunctionDefinitionNames.get qualifier with
| Some hit -> hit
| None -> (
let () = produce_and_write_aside read_only qualifier in
match FunctionDefinitionNames.get qualifier with
| Some hit -> hit
| None -> failwith "Should be impossible")


let function_definition read_only ?dependency:_ function_name =
match SingleFunctionDefinition.get function_name with
| Some hit -> hit
| None -> (
let () = produce_and_write_aside_function_definitions_all_parents read_only function_name in
match SingleFunctionDefinition.get function_name with
| Some hit -> hit
| None ->
SingleFunctionDefinition.write_around function_name None;
None)
end
module FunctionDefinitionReadOnly = struct
include FunctionDefinitionsTable.ReadOnly

module DependencyTrackedLogic = struct
let function_definitions_of_qualifier read_only ?dependency qualifier =
ReadOnly.get read_only ?dependency qualifier
get read_only ?dependency qualifier


let define_names_of_qualifier read_only ?dependency qualifier =
let function_definitions_of_qualifier =
function_definitions_of_qualifier read_only ?dependency
in
OutgoingDataComputation.define_names_of_qualifier function_definitions_of_qualifier qualifier
function_definitions_of_qualifier read_only ?dependency qualifier
>>| Reference.Map.Tree.keys
|> Option.value ~default:[]


let function_definition read_only ?dependency function_name =
let function_definitions_of_qualifier =
function_definitions_of_qualifier read_only ?dependency
in
OutgoingDataComputation.function_definition function_definitions_of_qualifier function_name
end

module FunctionDefinitionReadOnly = struct
include FunctionDefinitionsTable.ReadOnly

let track_dependencies read_only =
let { Configuration.Analysis.track_dependencies; _ } =
controls read_only |> EnvironmentControls.configuration
in
track_dependencies


let define_names_of_qualifier read_only =
if track_dependencies read_only then
DependencyTrackedLogic.define_names_of_qualifier read_only
else
FineGrainedUntrackedLogic.define_names_of_qualifier read_only


let function_definition read_only =
if track_dependencies read_only then
DependencyTrackedLogic.function_definition read_only
else
FineGrainedUntrackedLogic.function_definition read_only


let attribute_resolution = upstream_environment
Expand Down
6 changes: 6 additions & 0 deletions source/analysis/functionDefinitionEnvironment.mli
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@
module FunctionDefinitionReadOnly : sig
include Environment.ReadOnly

val function_definitions_of_qualifier
: t ->
?dependency:SharedMemoryKeys.DependencyKey.registered ->
Ast.Reference.t ->
FunctionDefinition.t Ast.Reference.Map.Tree.t option

val define_names_of_qualifier
: t ->
?dependency:SharedMemoryKeys.DependencyKey.registered ->
Expand Down

0 comments on commit e8df7ec

Please sign in to comment.