[m-rev.] for review: threadsafe version_arrays in Java

Paul Bone pbone at csse.unimelb.edu.au
Mon Apr 19 12:00:22 AEST 2010


On Fri, Apr 16, 2010 at 04:11:02PM +1000, Peter Wang wrote:
> 
> Estimated hours taken: 12 + some
> Branches: main, 10.04
> 
> Make the version_array and version_hash_table types thread safe in the C and
> Java grades.
> 
> @@ -639,6 +639,25 @@ extern ML_va_ptr    ML_va_rewind(ML_va_ptr);
> 
>  #define ML_va_latest_version(VA)   ((VA)->index == -1)
> 
> +#ifdef MR_THREAD_SAFE
> +    #define ML_maybe_lock(lock)                         \
> +        do {                                            \
> +            if (lock) {                                 \
> +                MR_LOCK(lock, ""ML_maybe_lock"");       \
> +            }                                           \
> +        } while (0)
> +
> +    #define ML_maybe_unlock(lock)                       \
> +        do {                                            \
> +            if (lock) {                                 \
> +                MR_UNLOCK(lock, ""ML_maybe_unlock"");   \
> +            }                                           \
> +        } while (0)
> +#else
> +    #define ML_maybe_lock(lock)     ((void) 0)
> +    #define ML_maybe_unlock(lock)   ((void) 0)
> +#endif
> +
>  ML_va_ptr
>  ML_va_get_latest(ML_va_ptr VA)
>  {

In asm_fast.par it would be better if the current context yeilded, freeing it's
engine to do some other work rather than locking the entire OS thread.  This is
something I'd like to do over the whole runtime and standard library, it dosn't
need to be addressed in this patch.

In pratice, this shouldn't cause a problem such as a deadlock, since all the
context-switching in asm_fast.par is not-premptive, so a thread that might hold
this lock will probably never be descheduled.

> @@ -678,6 +731,23 @@ ML_va_get(ML_va_ptr VA, MR_Integer I, MR_Word *Xptr)
>  }
> 
>  int
> +ML_va_set_dolock(ML_va_ptr VA0, MR_Integer I, MR_Word X, ML_va_ptr *VAptr)
> +{
> +#ifdef MR_THREAD_SAFE
> +    MercuryLock *lock = VA0->lock;
> +#endif
> +    int         ret;
> +
> +    ML_maybe_lock(lock);
> +
> +    ret = ML_va_set(VA0, I, X, VAptr);
> +
> +    ML_maybe_unlock(lock);
> +
> +    return ret;
> +}
> +
> +static int
>  ML_va_set(ML_va_ptr VA0, MR_Integer I, MR_Word X, ML_va_ptr *VAptr)
>  {
>      ML_va_ptr VA1;
> @@ -691,6 +761,9 @@ ML_va_set(ML_va_ptr VA0, MR_Integer I, MR_Word X,
> ML_va_ptr *VAptr)
>          VA1->index      = -1;
>          VA1->value      = (MR_Word) NULL;
>          VA1->rest.array = VA0->rest.array;
> +#ifdef MR_THREAD_SAFE
> +        VA1->lock       = VA0->lock;
> +#endif
> 
>          VA0->index     = I;
>          VA0->value     = VA0->rest.array->elements[I];

I'm concerned about the copying of this lock.  Is it a copy or do VA0->lock and
VA1->lock both refer to the same lock.  VA0->lock is currently locked and will
be unlocked by ML_va_set_dolock.  Will this unlock VA1->lock?  (which it
should).

The rest is fine.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20100419/41a8164d/attachment.sig>


More information about the reviews mailing list