[ruby-core:71229] Re: [Ruby trunk - Feature #11625] [Open] Unlock GVL for SHA1 calculations

[e-mail] wrote:
> --- a/ext/digest/digest.c
> +++ b/ext/digest/digest.c
> @@ -624,6 +624,7 @@ rb_digest_base_update(VALUE self, VALUE str)
>      TypedData_Get_Struct(self, void, &digest_type, pctx);
>  
>      StringValue(str);
> +    str = rb_str_dup(str);
>      algo->update_func(pctx, (unsigned char *)RSTRING_PTR(str), RSTRING_LEN(str));
>      RB_GC_GUARD(str);

Better to use rb_str_new_frozen instead of rb_str_dup
to prevent string modification by users traversing ObjectSpace.
IO#syswrite and similar methods do this.

Maybe rb_obj_hide + rb_gc_force_recycle is worth it for garbage
reduction, too.

> --- a/ext/digest/sha1/sha1.c
> +++ b/ext/digest/sha1/sha1.c

> +void SHA1_UpdateNoGVL(SHA1_CTX *context, const uint8_t *data, size_t len)
> +{
> +    struct unpack_nogvl args;
> +    args.context = context;
> +    args.data = data;
> +    args.len = len;
> +
> +    rb_thread_call_without_gvl(SHA1_UpdateNoGVLUnpack, &args, RUBY_UBF_PROCESS, NULL);

I don't think there is any need to use a UBF, here.
Having a UBF could even be dangerous by corrupting the state with
no resume point, allowing incorrect checksums to be generated.

s/RUBY_UBF_PROCESS/NULL/ also brings you under the 80-column limit :)

> +int SHA1_FinishNoGVL(SHA1_CTX* context, uint8_t digest[20])
> +{
> +    struct finish_nogvl args;
> +    args.context = context;
> +    args.digest = digest;
> +
> +    rb_thread_call_without_gvl(SHA1_FinishNoGVLUnpack, &args, RUBY_UBF_PROCESS, NULL);

ditto

> diff --git a/ext/digest/sha1/sha1.h b/ext/digest/sha1/sha1.h
> index 6f1c388..030d59c 100644
> --- a/ext/digest/sha1/sha1.h
> +++ b/ext/digest/sha1/sha1.h
> @@ -31,6 +31,8 @@ void	SHA1_Transform _((uint32_t state[5], const uint8_t buffer[64]));
>  int	SHA1_Init _((SHA1_CTX *context));
>  void	SHA1_Update _((SHA1_CTX *context, const uint8_t *data, size_t len));
>  int	SHA1_Finish _((SHA1_CTX *context, uint8_t digest[20]));
> +void	SHA1_UpdateNoGVL _((SHA1_CTX *context, const uint8_t *data, size_t len));
> +int	SHA1_FinishNoGVL _((SHA1_CTX *context, uint8_t digest[20]));

We can probably make SHA1_Update, SHA1_Finish (and perhaps other
functions, such as SHA1_Transform) static, now.