Targetted optimisations #870

jonludlam · 2022-06-13T10:17:10Z

Building on the other PRs, this one contains a set of optimisations.

Julow

This is huge!

Julow · 2022-06-15T15:15:48Z

src/model/paths_types.ml

@@ -1,128 +1,187 @@
 open Names
 (** {1 Paths} *)

+type 'a id = { iv : 'a; ihash : int; ikey : string }


Why store the hash instead of letting it be recomputed from the key ?

just so we only compute it once.

Julow · 2022-06-15T15:18:06Z

src/model/paths.mli

@@ -16,6 +16,8 @@



All these diffs make the Paths module even more annoying. What do you think of getting rid of it ?
It is not abstracting anything from Paths_types, it's just adding wrapper modules, which are not used much and could be written inline every time we want a Map or a Set.

Mmm, we could do this. It is certainly a bit annoying as it is. I'd like to hold off just a bit though.

Julow · 2022-06-15T15:19:40Z

src/model_desc/paths_desc.ml

@@ -60,95 +60,96 @@ module General_paths = struct

  let rec identifier : Paths.Identifier.t t =
    Variant


There's an opportunity to have a shorter output, this could return the key.

True, though I wasn't thinking about printing the keys when I made them so they might not be particularly legible. In particular they're backward, so Foo.bar is v_bar.m_Foo IIRC (I thought many identifiers would have the same prefix, so reversing them might make it quicker to reject when comparing them)

This would have no effect for hash and equal, only compare, which is used less.

Julow · 2022-06-15T15:22:13Z

src/xref2/component.ml

-  let compare a b = Ident.compare (a :> Ident.any) (b :> Ident.any)
+  let compare a b =
+    let i1 = match a with `LRoot (_, i) | `LModule (_, i) -> i in
+    let i2 = match b with `LRoot (_, i) | `LModule (_, i) -> i in


Does this really have an effect ? I'm fine with specializing cases where the type has only one constructor (TypeMap) but I would be surprised if this leads to a speed up compared to the overhead of the Map.

It does, quite a big effect.

In my testing, Ident.int_of_any compiles to just an argument access (doing just two loads, ignoring the constructor) and is inlined inside Ident.compare, which then inline the integer comparison (which is not optimized, it does two cmp).
The new versions seems to compiles to the same thing but then calls the polymorphic compare.

Do you have an input to benchmark this function ?

What I was doing was turning off all the caches and doing dune build @docgen with ODOC_BENCHMARK=true (and with core_kernel changed to core if you've got v0.15)

(jonludlam@074aa6a, jonludlam@0aa4240 and jonludlam@ec5b94d)

After your comment I was curious and went back to do some more testing. Looks like the performance benefit I initially saw was just a fluke I think. Averaged over 10 runs, before and after this change I got:

Before: 258.052 ±0.318
After: 257.672 ±0.427

I also ran the same job on all the subsequent commits and got:

Add smart constructors to identifiers and avoid polymorphic compare: 203.102 ±1.15
Simplify paths module: 206.368 ±1.016
Turn off caching in Of_Lang: 167.676 ±0.653
4.07/8 compatibility: 171.707 ±0.532 (this one is a bit annoying!)
Correct Identifier compare function: 175.155 ±0.685 (doh!)

I have amended the commit so all that is left is the removal of a couple of constructors that weren't used.

Julow · 2022-06-15T15:36:20Z

src/xref2/cpath.ml

@@ -186,16 +186,17 @@ and is_resolved_module_hidden :
 fun ~weak_canonical_test ->


Unrelated to this PR but weak_canonical_test is very rarely passed through, almost always set to a constant. Would it make sense to remove this argument but do extra check at the call site ? eg. change is_resolved_module_hidden ~weak_canonical_test:true p into is_weak_canonical p || is_resolved_module_hidden p,

I think I prefer it as one function - though we could make the weak_canonical_test an optional parameter.

Julow · 2022-06-15T15:43:41Z

src/model/paths.ml


-  let compare = compare
+  let compare x y = compare x.ihash y.ihash


This doesn't correspond to equal, should compare the key when the hashes matches.

Oh, good spot thanks!

Use map adds instead - this is much more efficient

Use Odoc_model.Paths.Path.*.t as the RHS of the cpath canonical constructors. These should always be absolute paths, so there should be no need to translate to/from cpaths

The problem here is that if the canonical path for a type contains a canonical module then we need to resolve that to ensure the identifiers are correct, which is required by the logic to select the 'minimal' canonical path. We didn't hit this in practise until the subsequent commit.

Explicitly unresolve the 'source' path, on the basis that we're way more likely to use the destination.

When paths don't use local idents, we keep them as Odoc_model.Paths.Path* to avoid unnecessary round trips.

This adds a string representation of the identifier that can be reliably compared in place of the polymorphic compare that was previously being used.

@Julow

Spotted in PR review by @Julow

jonludlam · 2022-06-22T10:58:47Z

For completeness, the timing stats for most of the commits:

master: 620s
Get rid of map merges: 300s
Change type of Cpath canonical constructor: 290s
Hashtbls rather than maps: 300s
Handle canonical modules whilst handling canonical types: 280s
Only the destination of aliases in now resolved: 250s
Add smart constructors to identifiers and avoid polymorphic compare: 203.102 ±1.15
Simplify paths module: 206.368 ±1.016
Turn off caching in Of_Lang: 167.676 ±0.653
4.07/8 compatibility: 171.707 ±0.532
Correct Identifier compare function: 175.155 ±0.685

first few were one run only so no stats.

Annoyingly 4.02 doesn't allow doc comments on type constructors

Julow

Pretty huge improvements ! Let's merge

Julow · 2022-06-29T14:13:25Z

dune

@@ -4,7 +4,7 @@
 (env
 (dev
  (flags
-   (:standard -g -w -18-53)))
+   (:standard -g -w -18-53-50)))


I think this should be disabled only in the module it's needed. It's a 4.02 compat thing so I'd prefer if it was less permanent.

@sabine

CHANGES: Additions - New unstable option `--as-json` for the HTML renderer that emits HTML fragments (preamble, content) together with metadata (table of contents, breadcrumbs, whether katex is used) in JSON format. (@sabine, ocaml/odoc#908) - New maths support via `{m ... }` and `{math ... }` tags. (@giltho, @gpetiot, ocaml/odoc#886) - Various optimisations (@jonludlam, ocaml/odoc#870, ocaml/odoc#883) - Better handling of alerts and deprecation notices. (@panglesd, ocaml/odoc#828) - Handle language tags on code blocks (@Julow, ocaml/odoc#848) Bugfixes - Shadowing issues (@jonludlam, ocaml/odoc#853) - Layout fixes and improvements (@panglesd, ocaml/odoc#832, ocaml/odoc#839, ocaml/odoc#847) - Handle comments on class constraints and inherit (@Julow, ocaml/odoc#844) - Disable the missing root warning (@jonludlam, ocaml/odoc#881)

jonludlam force-pushed the targetted-optimisations branch 4 times, most recently from 1c49fe8 to a76015d Compare June 14, 2022 13:33

Julow reviewed Jun 15, 2022

View reviewed changes

jonludlam force-pushed the targetted-optimisations branch from a76015d to df5ac5b Compare June 22, 2022 10:37

jonludlam added 11 commits June 22, 2022 11:37

Get rid of map merges

5a6cb23

Use map adds instead - this is much more efficient

Change type of Cpath canonical constructor

bd60f72

Use Odoc_model.Paths.Path.*.t as the RHS of the cpath canonical constructors. These should always be absolute paths, so there should be no need to translate to/from cpaths

Hashtbls rather than Maps in Lang<->Component

587a003

Only the destination of Alias paths is now resolved

d371800

Explicitly unresolve the 'source' path, on the basis that we're way more likely to use the destination.

Add 'gpaths' - global paths - to cpaths

04ecc0b

When paths don't use local idents, we keep them as Odoc_model.Paths.Path* to avoid unnecessary round trips.

Remove unused type constructors in Cpath

574f3bf

Add smart constructors to identifiers and avoid polymorphic compare

28d8bee

This adds a string representation of the identifier that can be reliably compared in place of the polymorphic compare that was previously being used.

Simplify paths module

4c05a41

Turn off caching in Of_Lang

f98e26f

4.07/8 compatibility

4f78f66

jonludlam force-pushed the targetted-optimisations branch from df5ac5b to 2418960 Compare June 22, 2022 10:38

Correct Identifier compare function

ca1d61c

Spotted in PR review by @Julow

jonludlam force-pushed the targetted-optimisations branch from 2418960 to ca1d61c Compare June 22, 2022 10:48

jonludlam added 2 commits June 22, 2022 12:03

Disable unattached documentation comment warning

36037e9

Annoyingly 4.02 doesn't allow doc comments on type constructors

4.02 compat

3f46aea

Julow approved these changes Jun 29, 2022

View reviewed changes

Better 4.02 compatibility

e1a20e7

jonludlam merged commit 704ce16 into ocaml:master Jun 29, 2022

jonludlam mentioned this pull request Dec 13, 2022

[new release] odoc (2.2.0) ocaml/opam-repository#22656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Targetted optimisations #870

Targetted optimisations #870

jonludlam commented Jun 13, 2022

Julow left a comment

Julow Jun 15, 2022

jonludlam Jun 22, 2022

Julow Jun 15, 2022

jonludlam Jun 22, 2022

Julow Jun 15, 2022

jonludlam Jun 22, 2022

Julow Jun 29, 2022

Julow Jun 15, 2022

jonludlam Jun 15, 2022

Julow Jun 16, 2022

jonludlam Jun 16, 2022

jonludlam Jun 16, 2022

jonludlam Jun 22, 2022

jonludlam Jun 22, 2022

Julow Jun 15, 2022

jonludlam Jun 22, 2022

Julow Jun 15, 2022

jonludlam Jun 16, 2022

jonludlam Jun 22, 2022

jonludlam commented Jun 22, 2022 •

edited

Loading

Julow left a comment

Julow Jun 29, 2022

		@@ -60,95 +60,96 @@ module General_paths = struct

		let rec identifier : Paths.Identifier.t t =
		Variant

		@@ -186,16 +186,17 @@ and is_resolved_module_hidden :
		fun ~weak_canonical_test ->


		let compare = compare
		let compare x y = compare x.ihash y.ihash

Targetted optimisations #870

Targetted optimisations #870

Conversation

jonludlam commented Jun 13, 2022

Julow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonludlam commented Jun 22, 2022 • edited Loading

Julow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonludlam commented Jun 22, 2022 •

edited

Loading