#27680 closed enhancement (fixed)

MR11: Lazy initialization of libgap

Reported by: galois Owned by:
Priority: major Milestone: sage-8.8
Component: interfaces Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: e687ee6 (Commits) Commit: e687ee6dc0468a40dcf84e41cc60e99d260d662f
Dependencies: Stopgaps:

Description (last modified by embray)

  1. Madison Bray (@embray) opened a merge request at https://gitlab.com/sagemath/sage/merge_requests/11:

Previously the Gap class that implements the (effectively singleton) libgap instance variable in sage.libs.gap.libgap called initialize() (the function which initializes libgap for the process) in its __init__ method.

Because of this, it was impossible to import it without causing libgap to be initialized, creating slowdown during Sage initialization. The tradeoff is that most code in Sage that uses the libgap interface has to awkwardly use inline imports of from sage.libs.gap.libgap import libgap all over the place.

I would like to do away with that, especially for work on #18267.

This removes initialize() from Gap.__init__() and instead carefully places calls to initialize() just in the few places where it's absolutely crucial to ensure libgap is initialized first (specifically in code paths that users and developers are actually intended to use directly; it is not added directly to every single function that uses GAP objects).

This sacrifices some simplicity in implementation of the libgap interface for simplicity in using it, which I think is crucial for updating more code in Sage to use it over the pexpect interface.

Change History (5)

comment:1 Changed 14 months ago by embray

  • Component changed from PLEASE CHANGE to interfaces
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 14 months ago by galois

  • Commit changed from a756429e85c1485d697d1cfc76605289b30e5896 to e687ee6dc0468a40dcf84e41cc60e99d260d662f

New commits added to merge request. I updated the commit SHA-1. This was a forced push. New commits:

de4332bChange libgap initialization to work lazily. Don't initialize() in Gap.__init__ but only just before libgap actually needs to be initialized.
e687ee6This test no longer makes sense in light of lazy initialization of libgap

comment:3 Changed 14 months ago by embray

  • Authors changed from E. Madison Bray to Erik Bray

Apparently my gitlab display name is not the same as my canonical name in Sage's build stuff. Perhaps we could add both.

comment:4 Changed 13 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good. send it to the bots.

comment:5 Changed 13 months ago by vbraun

  • Branch changed from u/galois/mrs/11/u/embray/libgap/lazy-initialization to e687ee6dc0468a40dcf84e41cc60e99d260d662f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.