chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.79k stars 421 forks source link

[Library Stabilization] BigInteger module status #17616

Closed lydia-duncan closed 3 months ago

lydia-duncan commented 3 years ago

The BigInteger module today provides locale support for GMP computations and operations on a bigint type (which is larger than a standard int/uint). This module does not take a strong position separating or unifying itself with the GMP module on which it relies. For stabilization of this library, there are three paths we could take:

  1. Merge the contents of this module back into the GMP module
  2. Make this module's API feel more independent of GMP, by adjusting the interface in ways that feel more "Chapelerific"
  3. "Both" of the above - merge the current contents of this module into GMP and make a new module (potentially with the same name while renaming the new submodule of GMP to be GMPBigInteger or something) with the more "Chapelerific" interface.

In the meeting, it sounded like there was generally a desire for a Chapelerific version, implying that option 2 or 3 would be the way to go.

Things to consider when deciding:

lydia-duncan commented 3 years ago

React to this comment if we should move BigInteger back into the GMP module

lydia-duncan commented 3 years ago

React to this comment if we should make BigInteger more independent of GMP

lydia-duncan commented 3 years ago

React to this comment if we should move the current version of BigInteger back into the GMP module and make a separate version that is more Chapelerific (name TBD)

mstrout commented 3 years ago

Of the three options, I prefer moving the BigInteger back into the GMP module and having submodules to separate between the original GMP interface and the various subcategories of functionality (eg., random, math stuff like Legendre) that have been Chapelized to enable use in distributed structures.

However, I thought there was another option mentioned of keeping the GMP module as a single-local module that no longer has any dependence on BigInteger by moving the GMPRandom stuff into BigInteger (with appropriate name changes). This is actually my top choice.

Why?

  1. We would have a cleaner explanation to users as to which module they should choose. One if they want to use BigIntegers in distributed data structures and the other if they were doing local stuff and didn't want to write existing code that used fancy GMP stuff.
  2. Having two modules would help in this case where there is such a large amount of functionality.
  3. There are some cases where circular dependences are unavoidable, but I don't see this as being one of them.
dlongnecke-cray commented 3 years ago

I voted for moving bigint back into GMP and making a new version that is implementation agnostic at some later point in time. Michael made it sound as though it would be a fairly ambitious endeavor to separate the interface of bigint as it stands today from GMP. We could probably reach that point faster if we just created a new bigint instead of modifying the old one in place.

mppf commented 3 years ago

One thing that didn't make it into this issue is that the bigint type is designed to expose the fact that GMP is used to implement it. That way, if a user finds that they want to call a GMP function directly, either because of a missing feature or because of something not performing the way they want, they can. So, I think of it as a "multiresolution design" library. I think that this is an important feature of the library. However, I do not know to what degree users of Chapel care about it.

Anyway, the downside of this approach is that it ties bigint to GMP. Maybe that is fine for the long term (because GMP is actually quite stable) but I get the sense that most of us do not view it that way, and would like bigint to be separable.

lydia-duncan commented 3 years ago

That's a good point. However, I don't think it would be a lot of effort to change the type such that it appears less reliant on GMP from the outside (without changing much of the implementation). There is minimal exposure to types defined in the GMP module through the interface as presented today.

mppf commented 3 years ago

Right, but there is a lot of functionality that we wouldn't necessarily have were it not based on GMP. In particular I would expect that a simple-as-possible bigint type would just support the same things as int i.e. operators like+ += < ... . In particular the methods setting the receiver like bigint.add as well as methods like bigint.jacobi or bigint.divexact have both functionality and names driven by GMP.

So when I vote for

React to this comment if we should move the current version of BigInteger back into the GMP module and make a separate version that is more Chapelerific (name TBD)

I am expecting that the not-tied-to-GMP version wont have the almost all of the methods that are currently available on bigint. As a result I think creating it is mostly about defining the operators. And, these operators form a relatively obvious API.

Note also that we can arrange to allow a GMP bigint to be initialized from one of the not-tied-to-GMP ones.

vasslitvinov commented 3 years ago

To me, this is what we ultimately want (perhaps borrowing Michael's idea):

I prefer keeping "GMP integer" and "GMP interop" modules separate. I do not see the need to merge them.

For GMPRandom, I propose putting its GMP-only pieces into the "GMP interop" module and its currently-BigInteger-related pieces into the "GMP integer" module.

bradcray commented 3 years ago

I know I missed the deep-dive, so am behind on the discussion, but the notion of supporting two big integer types seems like overkill and a maintenance burden to me. I.e., as a user, I wouldn't want to have to choose between "this is the bigint type that is convenient, but doesn't provide a lot of functionality" vs. "this is the bigint type that isn't built-in but does everything I could want." And as a developer, I wouldn't want to have to develop, maintain, and test two distinct types without a clear reason. So what's the short story for why the Vass/Michael approach is desirable? What are we trying to fix? Do we realistically envision wanting to define a second bigint implementation distinct from GMP in the future? (AFAIK there's not a lot of competition in the arbitrary math space to GMP is there? I think most languages supporting bigint capabilities do so via GMP?)

Assuming there is a reason I'm not seeing, taking a page from the Regex and Sys discussions, I could imagine a CHPL_BIGINT setting which says "how is the bigint type within BigInteger implemented?" where today the only possible answers are "gmp" or "none". Today, if someone didn't have GMP, they'd have to select 'none'. In some future if there were a second implementation that was not GMP-based, they could select that. But it would still just be one type in the code rather than two.

I completely agree with making the names of the routines in BigInt less GMP-based and more Chapeltastic, but I don't see how we got from there to having a GMP-less bigint and a GMP-specific bigint type.

image

lydia-duncan commented 3 years ago

I think the best summary is that there's a worry that people that start by wanting to use the GMP module won't necessarily recognize that the BigInteger module provides a lot of helpful wrappers for what they'd want to do, so they'd end up reimplementing things like locality handling and potentially get it wrong

mppf commented 3 years ago

@bradcray - the current bigint intentionally exposes the GMP details in the interface. See e.g.

bigint.mpz bigint.get_limbn bigint.mpzStruct

These methods are available for the reason I described in this comment - (basically, multiresolution design). Additionally there are many elements of the interface that we would almost certainly not arrange the way they are were it not for GMP (see this comment) and/or think about methods like add and jacobi.

My understanding of "Make bigint more Chapeltastic" as a direction is that it necessarily would disallow the dependence on GMP to be visible and make the type and module only support the operations we'd deem essential. But that might not be what you are arguing. Are you saying that GMP is common and stable enough that it is OK if our bigint type and its interface depend on it, or something interface-compatible with it, for the forseeable future?

bradcray commented 3 years ago

people that start by wanting to use the GMP module

I don't generally feel like we should encourage people to use the GMP module, and so don't feel particularly worried about people who start there rather than looking for a first-class bigint type. I'd like to think of the GMP module increasingly as the C-level module that implements BigInteger, similar to how we have C_MPI or C_FFTW modules that are exposed to the user more nicely as MPI or FFTW (to that end, it might even be smart to rename the GMP module to C_GMP in order to emphasize that it's not meant to be a Chapeltastic interface to GMP). We could even put something into the documentation indicating that it's intended as a low-level expert library, not something for the typical user.

Are you saying that GMP is common and stable enough that it is OK if our bigint type depends on it, or something interface-compatible with it, for the forseeable future?

Personally, it wouldn't make me at all uncomfortable to say that in Chapel 2.0 our bigint type is strongly tied to GMP (or something that can do the same things), similar to how we currently say that our regex capabilities are strongly tied to RE2.

If people find it offensive to have aspects of the interface that can't easily be divorced from GMP (like the .mpz field and other cases you cite above) defined by the BigInteger type—and I can completely understand why they might—my counterproposal would be to move those methods into a BigIntGMP module that provided new tertiary methods and/or standalone functions on the single bigint type, with the understanding that that module would only work with a GMP-based implementation of bigint (if we ever had another one). We could even put those into the GMP module itself (though this might not make as much sense in my C_GMP world above).

We could take that idea even further and move all GMP-specific math methods on bigints into this distinct module, but that's an example of where I don't feel uncomfortable tying bigint to GMP. My prediction would be that we'll hit some sort of Chapel standard library 3.0 event long before we want a second implementation of bigint, so for convenience, I'd probably just rename them to use more Chapeltastic / less GMP-specific names and leave them in BigInteger; or else create a non-GMP-specific BigIntegerMath module containing the routines if that were considered better for some reason. But I still wouldn't introduce a second bigint type.

mppf commented 3 years ago

@bradcray - those sound like reasonable approaches to me, supposing that they address other people's concerns. I particularly like the idea of putting GMP-specific parts of the interface in a different module. However this probably wouldn't help with GMP-isms like the proc bigint.add(a, b) which does this = a + b.

But, then we would have a different discussion about module names. If bigint is tied to GMP then it seems it should be in a GMP module. Certainly, the current contents of GMP are mostly crufty C bits that aren't likely to be what people need to use. But we can move those to GMP_C (actual name TBD) and then have the high level interfaces bigint and GMPRandom in GMP.

A related question - if we accept that bigint is tied to GMP, then what is the rationale for having it be in a seemingly separate top-level module BigInteger ?

ronawho commented 3 years ago

Do we consider bigint to be a part of the language or just a module? We decided that regular expressions are a core part of the language (or maybe just something that any chapel implementation would need to support). Do we consider bigint to be along the same lines where any chapel implementation would need to support it?

If not, I think we could consider making bigint a mason package (that depends on gmp) and stop bundling gmp as a third-party library.

At least for me the discussion about bigint being tied to gmp only matters if we consider bigint to be a core part of Chapel and I wouldn't want to tie a hypothetical future implementation to gmp. If it's not core, I don't really have a strong preference for what we do and I agree with the notion that it's ok if it's tied to gmp.

lydia-duncan commented 3 years ago

Personally, I think it would be nice to ensure there was a strategy for handling arbitrarily large integers in the language. Probably in my ideal world it wouldn't be distinguishable from regular integers, but since there are different considerations about storage between the two, I think having them be distinct types is reasonable.

bradcray commented 3 years ago

To Elliot's question, I don't consider them part of the language currently, and would suggest we not try to make them part of the language in the 2.0 timeframe without good reason. Specifically, I think it'll take a nontrivial effort to do well and don't think it's on the critical path for anyone (examples that I'm thinking of: should we write them as int(*)? Should proc foo(x: int(?)) { ... } accept bigints?).

I think of them as something that we want to ensure are automatically included with any Chapel installation, similar to sin() or regex, which is why standard module seems right to me (I don't consider regex part of the language once we decided not to pursue a special regular expression literal, just more of a "batteries included" feature—"of course we've got regex and bigint!").

If bundling became painful, my head would go more towards "Could we lean on mason or spack more to satisfy our reliance on GMP?" rather than moving the feature out to a package module over that. The request that we lean more on homebrew packages might be a good baby step in that direction.

To Michael's points:

If bigint is tied to GMP then it seems it should be in a GMP module.

I don't agree with this. I think the best module names are the ones that clearly reflect what the module provides rather than how they provide it—for example, I'm glad that our module is named Regex rather than RE2. I think many more programmers will recognize / understand what a bigint is than will recognize GMP (and, it gives us the flexibility to swap in some other implementation that provides all the same stuff—or can be made to by hook or by crook—down the line if GMP ever fails us, similar to Regex and RE2).

However this probably wouldn't help with GMP-isms like the proc bigint.add(a, b) which does this = a + b.

This one feels sufficiently math-y / implementation-neutral to me that I'd be inclined to make it part of the "of course all bigints have this" interface ("for performance reasons") rather than consider it something GMP-specific.

If there were methods that dealt more with the GMP implementation than math-style operations (say "iterate over all memory buffers implementing this bigint"), those are the sorts of things I'd put into a BigIntGMP or BigIntGMPImpl or simply GMP (as opposed to C_GMP) style module if we were to add one.

lydia-duncan commented 3 years ago

If I'm understanding Michael correctly, he'd be in favor of the BigInteger contents being in a submodule of GMP (which could potentially live in a different file, but that's an implementation detail). Brad, does having it live in a submodule of GMP assuage some of those worries? Or does it still seem too "off in a corner I wouldn't expect to look into"?

bradcray commented 3 years ago

That still seems too off in a corner to me. I'd expect the modules to be named after general, reasonably self-descriptive features or concepts, and combined into broader (and similarly clearly named) parent modules when there were features that shared an obvious theme. So if anything, I might expect a BigMath module with BigInteger and BigFloat submodules. Whereas, to me, putting it in or under GMP disguises it for anyone who's not already familiar with GMP.

mppf commented 3 years ago

If I'm understanding Michael correctly, he'd be in favor of the BigInteger contents being in a submodule of GMP

I was trying to explore that option, but I actually don't have a strong opinion on this structure right now. What I do care more about is the "is bigint tied to GMP" question.

I'd expect the modules to be named after general, reasonably self-descriptive features or concepts, and combined into broader (and similarly clearly named) parent modules when there were features that shared an obvious theme.

Seems like a good goal.

From the conversation here, it sounds like we're considering a 4th option beyond the 3 that people reacted to above. I will see if I can summarize it.

npadmana commented 3 years ago

Hi all -- while this discussion has focused on the relationship of bigint with GMP, I just wanted to come back to something that @mppf alluded to and that I personally found somewhat awkward when using the bigint library. Most of the bigint calls are written as methods on bigint, but that can result in statements like result.invert(num1, num2) when what I mean is result = invert(num1, num2). I almost prefer the C style here of mpz_invert(result, num1, num2), since the former reads as invert as operating on result, while the latter is just a function call with return parameters.

I realize this makes writing the interface much more involved, but I thought I'd mention it within this discussion of stabilizing bigint.

mppf commented 3 years ago

@npadmana - that's interesting. In most cases we have been preferring methods but I can understand that a method to replace the receiver might be harder to read. We have also generally been preferring type methods to free functions.

For operators like when writing a.add(b, c) we could potentially write a = b + c and have the compiler turn that into a call to a new operator type bigint.=+(ref lhs:bigint, arg1:bigint, arg2:bigint) where the compiler would turn a = b + c into +=(a, b, c). (There are other ways to handle this too).

But for something like invert we won't have an operator and then this question of function-or-method is very sensible. I suppose we could have generic handling for =something functions, and then we could write result = invert(num1, num2) perhaps, and the compiler would turn it into a call =invert(result, num1, num2).

Anyway this =+ / =invert idea has two main drawbacks that I know of:

(There are also other ways of handling this problem to do with implicit conversions, FWIW.)

So, if you want mpz_invert(result, a, b); would you rather write:

lydia-duncan commented 3 years ago

Hi all -- while this discussion has focused on the relationship of bigint with GMP, I just wanted to come back to something that @mppf alluded to and that I personally found somewhat awkward when using the bigint library. Most of the bigint calls are written as methods on bigint, but that can result in statements like result.invert(num1, num2) when what I mean is result = invert(num1, num2). I almost prefer the C style here of mpz_invert(result, num1, num2), since the former reads as invert as operating on result, while the latter is just a function call with return parameters.

I realize this makes writing the interface much more involved, but I thought I'd mention it within this discussion of stabilizing bigint.

Thanks, Nikhil. This was something that had bothered me when reviewing the module as a whole as well - it's done because of performance concerns for making new bigint instances, but I agree that it's clunky and not what you'd expect to have happen based on how Chapel code is typically written. I'm hoping to open an issue about this question specifically, after we've solidified how closely BigInteger and its interface is tied to GMP

bradcray commented 3 years ago

We have also generally been preferring type methods to free functions.

FWIW, I don't feel at all allergic to free functions now that we have import clauses. I think we've found good cases for type methods, but haven't internalized our internal conversations as concluding "free functions are to be avoided."

[edit: Specifically, in an import-oriented world, using type functions requires saying things like BigInteger.bigint.invert() when BigInteger.invert() seems plenty safe / specific / verbose already for those who want it. And I think those who use use get what they ask for. As a familiar example, I'd never want to have to type Math.real.sin(x); rather than Math.sin(x); or sin(x);.]

bradcray commented 3 years ago

it's done because of performance concerns for making new bigint instances

Just to clarify, I think that result = invert(num1, num2); is the case that would have a performance concern, but that invert(result, num1, num2); would not, right? (i.e., that first argument should be able to be handled just as efficiently as a formal argument as it would if it were this).

lydia-duncan commented 3 years ago

it's done because of performance concerns for making new bigint instances

Just to clarify, I think that result = invert(num1, num2); is the case that would have a performance concern, but that invert(result, num1, num2); would not, right? (i.e., that first argument should be able to be handled just as efficiently as a formal argument as it would if it were this).

That's correct, yeah!

npadmana commented 3 years ago

A few thoughts/responses --

[edit: Specifically, in an import-oriented world, using type functions requires saying things like BigInteger.bigint.invert() when BigInteger.invert() seems plenty safe / specific / verbose already for those who want it. And I think those who use use get what they ask for. As a familiar example, I'd never want to have to type Math.real.sin(x); rather than Math.sin(x); or sin(x);.]

I completely agree here. I think import gives the right degree of safety/verbosity/clarity -- adding in an additional type seems painful. And yes, please let's never write Math.real.sin(x). :-)

I'm intrigued by @mppf's thought about having the compiler automatically rewrite res = invert(...) into =invert(...), in part because I can imagine also doing things like this for matrices/vectors. That being said, I do think it introduces complexity into the entire process, with the danger of unintended consequences/interactions. Ultimately, to me, invert(res, a, b) isn't harder to write. It's a little less obvious to read that res=invert(a,b), but has the advantage that it is actually similar to the GMP version so there is less of an impedance mismatch.

mstrout commented 3 years ago

For one of Michael's comments above (https://github.com/chapel-lang/chapel/issues/17616#issuecomment-830415312) I see that 4 people gave a thumbs up to the idea of having 3 modules: BigInteger, GMPBigInteger, and GMP. Thinking from the perspective of a user, I want to minimize the number of modules I have to choose between using. Currently, I am not seeing enough support in the discussion for having 3 separate modules. I see the support for having 2. It would be better to have the interoperability one be called GMP or C_GMP so that people looking to use GMP specifically can easily find it. It is good to have a BigInteger module so that people not familiar with the lore that is GMP can represent and manipulate big integers.

The argument for having a separate GMPBigInteger library appears to be to provide GMP functionality for the bigint type. Some of the bigint capabilities such as the mpz field were also mentioned. However, could we take things like mpz_addmul and change them in BigInteger so they that their naming is not tied to GMP? IOW change

mpz_addmul(a.mpz, b.mpz, c.mpz);

to

addmul(a, b, c);

Then we would have the GMP module that is an interoperability module and the BigInteger module that was kind of like Regex (tied to RE2 in the functionality provided but not exposing the innards of the RE2 data structures).

Can we have 2 modules OR can someone better explain why 3 is so much better that it will be worth the additional learning curve?

lydia-duncan commented 3 years ago

The argument for having a separate GMPBigInteger library appears to be to provide GMP functionality for the bigint type. Some of the bigint capabilities such as the mpz field were also mentioned. However, could we take things like mpz_addmul and change them in BigInteger so they that their naming is not tied to GMP?

I'm not sure I'm following, I think there's a disconnect here. I think we can divide the contents of the BigInteger module into two concepts:

  1. Functionality that we'd expect to naturally want on a bigint type. This includes things like your addmul, etc, where the name can be made to not tie directly to the GMP naming scheme. While we could put a mpz_addmul version in the GMPBigInteger module, it's not the main motivator for having that module.
  2. Aspects of the type that reveal the underlying implementation. For instance, there's an mpzStruct() method today that I'd expect to put in a GMPBigInteger module if we had one. This category is what motivates the third module - it's probably useful to have it, but it is very tied to the implementation and if we want bigint to feel more independent of GMP, having it around isn't conducive to that effort. If we really didn't like the idea of having a GMPBigInteger module, this stuff would probably be "no doc"d or private when we support private methods. But one could argue that it is useful if you live in that world where you care that bigint is implemented by GMP under the covers. And if it's useful, then it should be documented and available.
mppf commented 3 years ago

FWIW I currently think it'd be sufficient to have 2 modules (but this is OK with me due to the idea that "in the long term, bigint will be implemented with GMP or something interface-compatible" which maybe some people are not OK with). In particular:

lydia-duncan commented 3 years ago

Moving the GMP bits of BigInteger into GMP would solidify a circular dependency between those modules

mppf commented 3 years ago

Right, but that doesn't bother me. But if it did bother anybody it can be solved by adding a GMP_C submodule.

lydia-duncan commented 3 years ago

Michelle asked for a breakdown of the parts that exposure the GMP functionality and how they are used today.

I believe these are the only aspects we would be worried about.

lydia-duncan commented 3 years ago

Based on this discussion and the voting, it sounds like the plan is:

Thanks for the discussion everyone!

bradcray commented 3 years ago

When we started talking about potentially making a BigIntegerGMPLib module that was distinct from BigInteger and had GMP-specific stuff in it, I was imagining we were talking about potentially dozens of things. Now that I know we're only talking about 2 routines (right?), I feel like maybe we should just leave them where they are and mark them as being part of an "expert / could potentially change" interface / maybe add a --warn-unstable warning for them. Specifically, I think that—as part of our multiresolution strategy—there ought to be some way of dropping into the guts of the GMP type to get at the underlying data (similar to how one might want to poke into the guts of the array implementation and get the C pointer to the buffer(s) implementing their array). So I'd be inclined not to no doc it.

Another approach would be to rename mpzStruct() to something like getImplDetails() or getGuts() and note that the behavior depends on the implementation of bigint (noting that, in our current GMP-based implementation, it will return an mpz structure). That would make it less GMP-specific without taking away the ability to get at the field.

If the numLimbs call can be made using the GMP module on the results of mpzStruct() directly, I'd just deprecate it from BigInteger and have people call the GMP routine when they want that information.

mppf commented 3 years ago

I think it'd be sensible to move these handful of routines to GMP and the extern declarations in GMP into a GMP_C submodule. (That would both address the multiresolution point and break the circular dependencies). If there is a big problem with that, I don't know what it is.

Edit -- Related to that, I think of bigint as the reasonable Chapel interface into GMP's mpz type. As a result, I think it'd be reasonable for use GMP to make bigint available.

So, I would structure the modules like this:

 module BigInteger (separate by popular demand)
   private use GMP.GMP_C

   bigint

module GMP
   public use GMP_C
   public use BigInteger

   GMPRandom
   bigint.mpzStruct etc

   (sub) module GMP_C
       lots of extern C declarations
bradcray commented 3 years ago

Am I correct that if the public use BigInteger in Michael's sketch above was just turned into use BigInteger, it would still work? If so, I personally wouldn't make GMP publicly re-export BigInteger. It makes me imagine this conversation:

"What does this BigInteger module do?" "It provides bigint" "What does this GMP module do?" "It also provides bigint and a bunch of other stuff." "Ah, so GMP is like a higher-level module above BigInteger?" "No..."

Put another way, I don't think this program should compile:

use GMP;  // "Give me a Chapeltastic view of GMP-related stuff"
var x: bigint;  // "Where'd this come from?"

(FWIW, I'm also not offended by circular dependences in modules, if that's relevant here).

lydia-duncan commented 3 years ago

A few thoughts/responses --

[edit: Specifically, in an import-oriented world, using type functions requires saying things like BigInteger.bigint.invert() when BigInteger.invert() seems plenty safe / specific / verbose already for those who want it. And I think those who use use get what they ask for. As a familiar example, I'd never want to have to type Math.real.sin(x); rather than Math.sin(x); or sin(x);.]

I completely agree here. I think import gives the right degree of safety/verbosity/clarity -- adding in an additional type seems painful. And yes, please let's never write Math.real.sin(x). :-)

I'm intrigued by @mppf's thought about having the compiler automatically rewrite res = invert(...) into =invert(...), in part because I can imagine also doing things like this for matrices/vectors. That being said, I do think it introduces complexity into the entire process, with the danger of unintended consequences/interactions. Ultimately, to me, invert(res, a, b) isn't harder to write. It's a little less obvious to read that res=invert(a,b), but has the advantage that it is actually similar to the GMP version so there is less of an impedance mismatch.

@npadmana - I opened #17699 for this issue (though I didn't move all the comments from here over there, I just linked to them)

lydia-duncan commented 3 years ago

I also forgot about a few other "implementation-y" functions, see #17702

bradcray commented 2 years ago

Commenting here since this looks like the closest we might have to an umbrella task for BigInt. In reviewing pidigits codes today, we noticed that big integers seem to be built into Julia, such that that might represent an interesting place to look for what names another modern language uses for these methods.

lydia-duncan commented 3 months ago

Closing, we've stabilized the BigInteger module