New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TSan] Interlock and unlock MonoJitStats #5437

Merged
merged 2 commits into from Sep 11, 2017

Conversation

Projects
None yet
4 participants
@cherusker
Contributor

cherusker commented Aug 24, 2017

This is the first part of telling TSan what to do with MonoJitStats; I dealt with all the gint32 values and partly touched the gboolean value (see FIXME). However, MonoJitStats also contains gdouble which can (1) probably be interlocked and (2) will be updated in a later step, together with the FIXME, as soon as an updated version of the atomic interface exists.

I did my best to distinguish between perf. critical parts and parts where Interlocked* () seems to work perfectly fine. As before, if I misjudged anything, I would kindly ask to comment the affected operations, and I will update them accordingly.

More (technical) details can be found in the commit messages.

@cherusker cherusker requested review from kumpera, lewurm and vargaz as code owners Aug 24, 2017

@dnfclas

This comment has been minimized.

dnfclas commented Aug 24, 2017

@cherusker,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

mono_jit_stats.max_ratio_method = g_strdup_printf ("%s::%s)", method->klass->name, method->name);
/* FIXME: use an explicit function to read booleans */
if ((gboolean)InterlockedRead ((gint32*)&mono_jit_stats.enabled)) {
InterlockedAdd (&mono_jit_stats.allocated_code_size, cfg->code_len);

This comment has been minimized.

@lewurm

lewurm Sep 6, 2017

Member

isn't that a semantic change? now allocated_code_size gets increased only if mono_jit_stats.enabled is set. before it was unconditional.

This comment has been minimized.

@cherusker

cherusker Sep 6, 2017

Contributor

You're right of course, that slipped through! Technically, I cannot think of a good reason to increase the counter while the stats are disabled but I will revert that, just in case ... lots of interesting stuff happened already while cleaning up the counters; no need to provoke the Mono spirits! :D

InterlockedWrite (&mono_jit_stats.biggest_method_size, code_size_ratio);
char *biggest_method = g_strdup_printf ("%s::%s)", method->klass->name, method->name);
biggest_method = InterlockedExchangePointer ((gpointer*)&mono_jit_stats.biggest_method, biggest_method);
g_free (biggest_method);

This comment has been minimized.

@lewurm

lewurm Sep 6, 2017

Member

good catch

This comment has been minimized.

@cherusker

cherusker Sep 6, 2017

Contributor

Yup - double free and what not :)

cherusker added some commits Aug 24, 2017

Prepare for the upcoming updates of `MonoJitStats`
- update the `MonoJitStats` struct
- add `UnlockedAddDouble ()` to atomic.h
- change the signature of `mono_time_track_end ()` to accept `gdouble` instead of `double`;
    `mono_time_track_* ()` is almost exclusively used with `Unlocked* ()` and `gdouble` is a typedef for `double` anyways
Interlock and unlock load/store operations that are connected to `Mon…
…oJitStats`

- use `gdouble` instead of `double` in connection with `mono_time_track_end ()`

@cherusker cherusker force-pushed the cherusker:cherusker-2017-08-24-mono-jit-stats branch from adcbb10 to ea4a8d7 Sep 6, 2017

@cherusker

This comment has been minimized.

Contributor

cherusker commented Sep 6, 2017

@lewurm just rebased and fixed #5437 (comment) ... I slightly rearranged the calls but the overall semantics stay the same.

@lewurm

lewurm approved these changes Sep 7, 2017

@lewurm

This comment has been minimized.

Member

lewurm commented Sep 7, 2017

@monojenkins squash

@monojenkins

This comment has been minimized.

Contributor

monojenkins commented Sep 7, 2017

cannot squash:

  • "Linux i386" state is "success"
  • "Linux x64" state is "success"
  • "Linux ARMv7 hard float" state is "success"
  • "Linux AArch64" state is "success"
  • "OS X i386" state is "success"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "failure"
4 similar comments
@monojenkins

This comment has been minimized.

Contributor

monojenkins commented Sep 8, 2017

cannot squash:

  • "Linux i386" state is "success"
  • "Linux x64" state is "success"
  • "Linux ARMv7 hard float" state is "success"
  • "Linux AArch64" state is "success"
  • "OS X i386" state is "success"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "failure"
@monojenkins

This comment has been minimized.

Contributor

monojenkins commented Sep 8, 2017

cannot squash:

  • "Linux i386" state is "success"
  • "Linux x64" state is "success"
  • "Linux ARMv7 hard float" state is "success"
  • "Linux AArch64" state is "success"
  • "OS X i386" state is "success"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "failure"
@monojenkins

This comment has been minimized.

Contributor

monojenkins commented Sep 9, 2017

cannot squash:

  • "Linux i386" state is "success"
  • "Linux x64" state is "success"
  • "Linux ARMv7 hard float" state is "success"
  • "Linux AArch64" state is "success"
  • "OS X i386" state is "success"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "failure"
@monojenkins

This comment has been minimized.

Contributor

monojenkins commented Sep 11, 2017

cannot squash:

  • "Linux i386" state is "success"
  • "Linux x64" state is "success"
  • "Linux ARMv7 hard float" state is "success"
  • "Linux AArch64" state is "success"
  • "OS X i386" state is "success"
  • "OS X x64" state is "success"
  • "Windows i386" state is "success"
  • "Windows x64" state is "failure"

@monojenkins monojenkins merged commit cf9dff4 into mono:master Sep 11, 2017

11 of 12 checks passed

Linux ARMv5 soft float Build finished. 46952 tests run, 1404 skipped, 1 failed.
Details
API Diff No public API changes found.
Details
Linux AArch64 Build finished. 46982 tests run, 1409 skipped, 0 failed.
Details
Linux ARMv7 hard float Build finished. 46952 tests run, 1404 skipped, 0 failed.
Details
Linux i386 Build finished. 46985 tests run, 1401 skipped, 0 failed.
Details
Linux x64 Build finished. 46985 tests run, 1403 skipped, 0 failed.
Details
Linux x64 FullAOT Build finished. 20910 tests run, 529 skipped, 0 failed.
Details
OS X i386 Build finished. 45742 tests run, 1284 skipped, 0 failed.
Details
OS X x64 Build finished. 45742 tests run, 1286 skipped, 0 failed.
Details
Test Result Viewer Click to view aggregated test results (Xamarin internal).
Details
Windows i386 Build finished. 42155 tests run, 1112 skipped, 0 failed.
Details
Windows x64 Build finished. 42177 tests run, 1114 skipped, 0 failed.
Details

@cherusker cherusker deleted the cherusker:cherusker-2017-08-24-mono-jit-stats branch Sep 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment