Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

very slow doctest for all_character_table_names (group_characters.jl:380-395) #2341

Open
benlorenz opened this issue May 4, 2023 · 10 comments · May be fixed by #3688
Open

very slow doctest for all_character_table_names (group_characters.jl:380-395) #2341

benlorenz opened this issue May 4, 2023 · 10 comments · May be fixed by #3688
Labels
bug Something isn't working

Comments

@benlorenz
Copy link
Member

I am currently running the doctests many times to identify some crashes and in the process added some statistics to my Documenter.jl checkout to print the location and duration of each block that is processed. And there is one doctest that takes almost half of the total time for the doctests on my machine:

page: Oscar.jl/src/Groups/group_characters.jl:380-395
289.550605 seconds (824.17 M allocations: 115.634 GiB, 3.44% gc time, 0.14% compilation time)

The total doctest time on that Linux machine is about 12 minutes.

Running the code from that doctest directly in the repl is fast:

julia> @time let 
              spor_names = all_character_table_names(is_sporadic_simple => true, is_duplicate_table => false);
              println(spor_names[1:5])
              spor_names = all_character_table_names(is_sporadic_simple, !is_duplicate_table; ordered_by = order);
              println(spor_names[1:5])
              length(all_character_table_names(number_conjugacy_classes => 1))
       end;
["B", "Co1", "Co2", "Co3", "F3+"]
["M11", "M12", "J1", "M22", "J2"]
  0.013375 seconds (113.00 k allocations: 2.989 MiB)

Any ideas if/how the runtime of this test could be influenced by some other doctests? @ThomasBreuer @fingolfin

I tried this in the CI as well and the duration for the doctests also went down by at least five minutes here: https://github.com/oscar-system/Oscar.jl/actions/runs/4884289287
14-19 min for the doctest step on ubuntu instead of something like 22-29.
(I just changed jldoctest to julia for that block: f81abbb)

Full log (the order in which the blocks are run seems very arbitrary): https://gist.github.com/benlorenz/c89099ffeb3a070ee4628ba2130cfcf8

@benlorenz benlorenz added the bug Something isn't working label May 4, 2023
@benlorenz
Copy link
Member Author

Interestingly this seems to be the same place as the crashes in #2336:

page: /home/datastore/lorenz/software/julia/Oscar.jl/src/Groups/group_characters.jl:380-395
Segmentation fault (core dumped)

@ThomasBreuer
Copy link
Member

The runtime of all_character_table_names depends on whether or not the GAP package Browse is available.
Without this package, the function is very slow. Oscar's __init__ tries to load the package into GAP (and to install it if it is not yet available), but currently Oscar starts also if this attempt fails.
I do not understand why the variant without the Browse package runs into a segmentation fault.

@fingolfin
Copy link
Member

Was this resolved by PR #2349 or is there still something to be done here?

@benlorenz
Copy link
Member Author

The doctest is disabled because it takes very long when Browse is not working (I don't really know why... I thought browse is just an ncurses wrapper), so I think this is waiting for GAP_pkg_Browse_jll.

@fingolfin
Copy link
Member

I think @ThomasBreuer once tried to explain to me why Browse (which indeed ought to "just" be an ncurses wrapper?) is making this character table code faster (and why whatever makes them faster can't be moved to ctbllib or GAP itself or...), but I don't think I ever fully understood... in any case, I forgot. So perhaps @ThomasBreuer could add a brief explanation here, then we have it for future reference :-)

@ThomasBreuer
Copy link
Member

According to the documentation of the Browse package, there are three levels of functionality:

  1. a low level interface to ncurses (the ncurses wrapper mentioned above),
  2. a medium level interface to the generic GAP function NCurses.BrowseGeneric,
  3. applications of these interfaces.

Some code for level 3. does not really depend on level 1., and part of this code is used in the CTblLib package.

Yes, one could reorganize the involved GAP packages such that all CTblLib functionality is available without Browse (more precisely: without the ncurses dependent part of Browse), for example by splitting off new GAP packages.
At the time when Browse, CTblLib, etc. were written, it was not usual to create many small packages.
In fact I had expected that the availability of Browse would not be a problem, and sooner or later it would become a package that is anyhow needed.

@fingolfin
Copy link
Member

I think this is also a problem for GAP users: I have seen many people who have a GAP with zero compiled packages. They then can't use Browse, but may still use ctbllib.

Logically I find it surprising that there is code in Browse that is "needed" for ctbllib to perform certain applications efficiently.

I don't know about how much code we are talking, perhaps as yet another alternative it could also simply be duplicated so that it is available if either package is used without the other?

@ThomasBreuer
Copy link
Member

Duplicating code is usually a good idea for creating problems, not for solving them.
But yes, I could copy the relevant parts from Browse to CTblLib. For that, new releases of both packages will be needed.

@fingolfin
Copy link
Member

@ThomasBreuer this should be resolved by the new CTblLib in GAP 4.13.0, right? (we are not using that in OSCAR, though, hence I am not closing this either way)

@ThomasBreuer
Copy link
Member

Yes, CTblLib 1.3.9 is independent of Browse w.r.t. the code needed by all_character_table_names.
(In the end, a new Browse release has not been necessary for that.)

@fingolfin fingolfin linked a pull request May 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants