Adding Random to standard libraries

[TobiasWrigstad]

Adds a new passive class "Random" to the standard libraries.
Each instance of Random encapsulates a seed of its own.
Technically, this may be wasteful -- possibly one seed per
thread might do, but this will facilitate debugging programs
with random numbers.

Documentation still lacking. Where should I put that?

[TobiasWrigstad]

@EliasC tells me there is no place for standard library documentation. Therefore, I proclaim this ready to be merged.

[supercooldave]

@TobiasWrigstad It still needs to be reviewed. No skipping steps.

[kikofernandez]

instead of relying on _this->... = ..., is it possible to write this.seed = embed ... end. we avoid relying on the internal name

[supercooldave]

Internal names are going to change real soon, so relying on them is a bad idea.

[TobiasWrigstad]

The thing is that I am storing an unsigned inside of an int64_t. Therefore I preferred to do it all at the C-level. What if the C code inserts a cast? Seems better to rely on local things only.
The other possibility is to have an embed type:

seed: embed unsigned end

but this will require adding an empty trace function. I could do that. But I would still need to do _this-> because "unsigned" is not a valid Encore type.

[supercooldave]

Don't you only need the cast when you are passing the seed to rand_r?

[TobiasWrigstad]

So,

  def init(seed:int) : void
    embed void
      _this->_enc__field_seed = (unsigned) #{seed};
    end

translates into

  ({_this->_enc__field_seed = (unsigned) _enc__arg_seed;});

whereas

  def init(seed:int) : void
    this.seed = embed int
      (unsigned) #{seed};
    end

translates into

  int64_t _embed_0 = ({(unsigned) _enc__arg_seed;
   });
  (*({ _this;}))._enc__field_seed = _embed_0;

I must confess that it is somewhat unclear to me what the consequences are of casting a signed int to an unsigned into and then into a signed int again, in this scenario. Does anyone have an idea?

I suspect this matters little in the end, as long as the behaviour of assigning seed N is consistent.

[TobiasWrigstad]

@supercooldave I don't think relying on internal names is a big problem — I suspect this is the module system messing with things? This is an easy fix once the changes are in.

[TobiasWrigstad]

@supercooldave Sorry, I meant ready to be reviewed. Precisely I mean, "I feel it can be merged to someone should review it".

[TobiasWrigstad]

@kikofernandez Can the commits be squashed during merge, or should I do that some other way?

[kikofernandez]

@TobiasWrigstad do you experience this same issue?

>>> FAILED TEST: random:
     | running out test...
     | encore/stdlib/random
     | Error: Module Random found in multiple places:
     | -- /Users/kikofernandezreyes/Code/encore/bundles/standard/Random.enc
     | -- ./Random.enc
     | Unable to determine which one to use.
     | ERROR: encore/stdlib/random.enc should compile.

[TobiasWrigstad]

@kikofernandez Yes. But not before. I wonder what I have changed. Could it be the random function and the Random class?

[kikofernandez]

@TobiasWrigstad we can squash the commits, you don't have to do anything

[supercooldave]

@kikofernandez @TobiasWrigstad The problem is that you have a file called random in both the local directory and one in the standard library, so when doing the import it doesn't know which one to import -- which is obviously a bug. For now, rename the test file to randomTest.enc.

[TobiasWrigstad]

I can't get rid of the error. This seems like something that @supercooldave should know how to fix/debug.

[TobiasWrigstad]

Does this have something to do with the case insensitive file system on the Mac?

[supercooldave]

@TobiasWrigstad Did you see my comment? Rename the test file to randomtest.enc or similar.

[TobiasWrigstad]

@supercooldave Thanks, I just saw. Test passes now!

@kikofernandez I don't know what you mean. The seed is not used in the code.

[kikofernandez]

never mind, I thought that something like this may work:

def random(max:int) : int                                                     |
28    let seed = this.seed in                                                     |
29    embed int                                                                   |
30      rand_r(#{seed}) % #{max};                                                 |
31    end

[kikofernandez]

but I don't know and need to embark now...

[albertnetymk]

https://github.com/TheGrandmother/Boids/blob/master/boids1/Random.enc

We had similar attempts during the hackathon in London.

[TobiasWrigstad]

@kikofernandez I see what you mean, but it won't work unless you write it like this:

def random(max:int) : int
  let
    result = 0
    seed = this.seed
  in {
    embed void
      #{result} = (int64_t) rand_r(&#{seed});
    end ; 
    this.seed = seed ;
    result 
  }

because there is a side-effect on seed that must be preserved or else we'll keep returning the same number. Do you think that code is preferable? There's going to be a slight performance degradation, but not much. Possibly none under -O3.

@EliasC Does inference work with embed types?

@albertnetymk — feel free to backport things into this PR if you think we need more bells and whistles.

[TobiasWrigstad]

@kikofernandez bump. See my rewrite of your code 4 days ago.

[kikofernandez]

@TobiasWrigstad regarding:

So,

def init(seed:int) : void
 embed void
   _this->_enc__field_seed = (unsigned) #{seed};
 end

translates into

({_this->_enc__field_seed = (unsigned) _enc__arg_seed;});

whereas

def init(seed:int) : void
  this.seed = embed int
    (unsigned) #{seed};
  end

translates into

int64_t _embed_0 = ({(unsigned) _enc__arg_seed;
 });
(*({ _this;}))._enc__field_seed = _embed_0;

I must confess that it is somewhat unclear to me what the consequences are of casting a signed int to an unsigned into and then into a signed int again, in this scenario. Does anyone have an idea?

I suspect this matters little in the end, as long as the behaviour of assigning seed N is consistent.

the thing is that we are doing the same cast in your original solution (first one above) given that the signature is:

void* _enc__method_Random__init(pony_ctx_t* _ctx, _enc__passive_Random_t* _this, int64_t _enc__arg_seed)

This makes me wonder if we should check on the limits, if (seed > 2^32) then return "error". For me, the reason for throwing the error is that a programmer may assume that he is passing a 64 bit integer when, in reality, we take the least 32 significant bits from the int64_t.

Consequence: this is not random anymore, big numbers now match small numbers and the random generator is not secure (to use in encryption apps). I think it's better to crash at runtime

[kikofernandez]

@TobiasWrigstad regarding:

@kikofernandez I see what you mean, but it won't work unless you write it like this:

def random(max:int) : int
  let
    result = 0
    seed = this.seed
  in {
    embed void
      #{result} = (int64_t) rand_r(&#{seed});
    end ; 
    this.seed = seed ;
    result 
  }

because there is a side-effect on seed that must be preserved or else we'll keep returning the same number. Do you think that code is preferable? There's going to be a slight performance degradation, but not much. Possibly none under -O3.

I do not like it at all... if makes it difficult to read. For this reason, I think it's better to keep your current implementation

[kikofernandez]

I think the PR is ready to merge as soon as we figure out the kind of behavior we want for the random numbers. This means, if a number is bigger than 32 bits, do we crash or do we do the cast and work on 32 bits?

[TobiasWrigstad]

@kikofernandez Re the 2^32 check. Excellent suggestion! Thanks!

[TobiasWrigstad]

@kikofernandez I've elected to check the seed but not the random function -- that will be too expensive in benchmarks that hit random a lot. Are you fine with that?

[kikofernandez]

@TobiasWrigstad the random function doesn't do any downcast, it's always 32-bits. So I think this is ok.
Merging PR in 30 min