Skip to content

Commit

Permalink
Use a fine-grained write-around cache if no dep tracking
Browse files Browse the repository at this point in the history
Summary:
**Idea**

We discovered in D58897396 that storing FunctionDefinition.t values
separately from ModuleComponents.t is helpful for performance; it
turns a small *loss* on this stack in Pyre into a small *win* and it
cuts the Pysa slowdown by a big factor.

But the Pysa perf is still not good; the RAM use is much higher than
before the stack, and the Callgraph computation is taking much longer
(presumably because it pushes into swap).

This diff attempts to mitigate the problem with a work-around: because
we know that Pysa only ever operates in a non-dependency-tracked
no-overlay setting, we can use global and non-dependency tracked
shared memory tables to power Pysa (and in fact to power pyre check
as well!).

This lets us use a simple write-around setup that is very similar to
the old UGE setup except that:
- it is much simpler because no incremental updates are needed
- there is much less code because we're only dealing with
   function definitions and none of the other module components

As evidence that it's simpler, I'll note that this diff adds about 120 lines
of extra complexity, whereas D58842854 removed about 1000 lines,
so this little hack is probably ~10x easier to maintain.

**Results**

*Abstractly, what's expected*

There's no change at all for incremental Pyre, although we don't
have good CI perf tests for that anyway. But for non-incremental
commands, which includes `pyre check`, `arc pyre check`, and
`pyre analyze`, we do have a change:

The extra RAM use caused by function definitions specifically that
was introduced in D58842854 should now be 100% eliminated,
although extra RAM use from loading entire module signatures (which
in the case of enormous classes, such as 100K-entry enums or
generated classes with hundreds of methods, can be substantial)
remains.

*Actual results on Pyre*

There's no discernable change in servicelab. This suggests that
just breaking out FunctionDefinitionEnvironment in D58897396
was enough to eliminate memory pressure and serialization of
function bodies as a major factor in Pyre; it's likely that the map
reduce for type check already had pretty good module-level
locality (as in functions from the same module tended to wind up
on the same worker).

*Actual results for Pysa*

This diff does seem to substantially improve on D58897396 (which
already improved on D58897396). In particular callgraph construction,
where the hit was coming from, drops from 55 minutes to 37 minutes;
this is still much longer than the 13 minutes on trunk but it is a big
help and the overall slowdown is only 22 minutes now.

From the memory explorer results...
- on base: https://fburl.com/unidash/huvaaaqz
- on diff: https://fburl.com/unidash/x6928gs7

...it still seems clear that there is memory pressure, we still spike to
about 57G instead of 47G which suggests we're probably still pushing
into swap, driving the callgraph slowdown, but I think we're swapping
*less* now.

**What else could we do for Pysa?**

*Shrink the worker pool for callgraph*

On trunk, callgraph construction is only around 13 minutes; on
this diff it is 37. If we were to halve the number of workers, we
might eliminate the memory pressure entirely and come out
with something like 25-30 minutes.

This is probably the simplest fix assuming it works, it trades
RAM use for wall time in a predictable way that is also easy
to adjust going forward if codebases continue to grow.

*Find and fix the cause of UGE "bigness"*

The results of D58842854 + D58897396 + D58905530 strongly
suggests that for some reason callgraph RAM use is actually being
driven by UGE memory (without function definitions). This is actually
surprising, since in most cases post-D58897396 the UGE data
would be relatively small, it's just unannotated globals + class
summaries.

The underlying problem is likely some combination of
- huge literal values in globals; we know that giant dicts and lists are
  not actually that unusual, some codebases put configurations into
  module globals
- huge class summaries, of which the most likely culprits are:
  - fat enums (we know the biggest ones are at least close to 100K
     members)
  - generated classes with hundreds of methods; if there are enough
     of these in generated codebases it could add up.

If we identify the problem, we could consider approaches to eliminate
it, for example setting size limits on unannotated global literals (and
treating bigger values as if they were a literal `...`).

This is likely something we should eventually do regardless,
but I'm not sure whether we want to dive into this kind of optimization
on the Pyre team right now given that the pyre perf is fine at the
moment and we really want to focus on 3.12, qualification, and
conformance.

*Mostly rejected: do the same atomization for UGE*

One option for this diff is to try to perform the same change for UGE.
I attempted this in D58908824, but there are some problems:
- the table structure is significantly more complex
   - probably as a result, it looks like my code is buggy; there are
      type checking errors
- even if I fix it, it's a bad change as anything more than a band-aid
   because the point of D58842854 is to allow us to start refactoring
   UGE so that the module components can depend on one another,
   and having to atomize them for Pysa means we can't actually do that.
   - as a result, I'm not sold on even doing the work to debug
      D58908824; why bother if we know it won't help long-term?

Note that I don't consider *this* diff to be a band-aid because it's
actually ok to atomize the function bodies if we analyze them one
at a time. Atomizing the signatures is a much bigger problem for
pyre architecture.

Reviewed By: migeed-z

Differential Revision: D58905530

fbshipit-source-id: b65e55f1f782c2c74c7b03129677e097054f5b1c
  • Loading branch information
stroxler authored and facebook-github-bot committed Jul 5, 2024
1 parent 4aa556b commit 6029143
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 12 deletions.
115 changes: 109 additions & 6 deletions source/analysis/functionDefinitionEnvironment.ml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ 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 @@ -92,24 +98,121 @@ end)

include FunctionDefinitionsTable

module FunctionDefinitionReadOnly = struct
include FunctionDefinitionsTable.ReadOnly
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 DependencyTrackedLogic = struct
let function_definitions_of_qualifier read_only ?dependency qualifier =
get read_only ?dependency qualifier
ReadOnly.get read_only ?dependency qualifier


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


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: 0 additions & 6 deletions source/analysis/functionDefinitionEnvironment.mli
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,6 @@
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 6029143

Please sign in to comment.