Great, thanks!
/Robbin
On 2019-10-07 18:41, Daniil Titov wrote:
Hi Robbin,
Yes, I ran my benchmark [1]. Please see below the output showing that the table
was grown.
../jdk/build/linux-x64-release/images/jdk/bin/java -cp .
-Xlog:thread+table=info ThreadStartupTest
Starting the test
[0.185s][info][thread,table] Grown to size:512
The test finished.
Execution time:15673 ms
[1] https://cr.openjdk.java.net/~dtitov/tests/ThreadStartupTest.java
Thanks!
Daniil
On 10/7/19, 12:34 AM, "Robbin Ehn" <robbin....@oracle.com> wrote:
Hi Daniil,
Yes, good, but:
>> >> Testing: Mach5 tier1, tier2, and tier3 tests successfully passed.
>> And if you have not done so, you should test this with the
benchmark you
>> have as
>> a stress test and see that this does what we think.
Can you please test it with your benchmark, if you have not done so?
/Robbin
>> Thanks, Robbin
>> >>
>> >> Thank you!
>> >>
>> >> Best regards,
>> >> Daniil
>> >>
>> >> On 10/2/19, 3:26 PM, "David Holmes" <david.hol...@oracle.com>
wrote:
>> >>
>> >> Hi Daniil,
>> >> On 3/10/2019 2:21 am, Daniil Titov wrote:
>> >> > Hi David and Robbin,
>> >> >
>> >> > Could we consider making the ServiceThread responsible
for the
>> >> ThreadIdTable resizing in the similar way how
>> >> > it works for StringTable and ResolvedMethodTable,
rather than
>> having
>> >> ThreadIdTable::add() method calling ThreadIdTable::grow()?
>> >> > As I understand It should solve the current issue and
>> address the
>> >> concern that the doing the resizing could be a relatively long
and
>> >> > doing it without polling for safepoints or while the
holding
>> >> Threads_lock is not desirable.
>> >> I originally rejected copying that part of the code from
the other
>> >> tables as it seems to introduce unnecessary complexity.
Having a
>> >> separate thread trying to grow the table when it could be
>> concurrently
>> >> having threads added and removed seems like it could
introduce
>> hard to
>> >> diagnose performance pathologies. It also adds what we
know to be a
>> >> potentially long running action to the workload of the
service
>> thread,
>> >> which means it may also impact the other tasks the service
thread is
>> >> doing, thus potentially introducing even more hard to
diagnose
>> >> performance pathologies.
>> >> So this change does concern me. But go ahead and trial it.
>> >> Thanks,
>> >> David
>> >> > Thank you,
>> >> > Daniil
>> >> >
>> >> >
>> >> > On 10/2/19, 6:25 AM, "David Holmes"
<david.hol...@oracle.com>
>> wrote:
>> >> >
>> >> > Hi Robbin,
>> >> >
>> >> > On 2/10/2019 7:58 pm, Robbin Ehn wrote:
>> >> > > Hi David,
>> >> > >
>> >> > >> What if the table is full and must be grown?
>> >> > >
>> >> > > The table uses chaining, it just means load
factor tip
>> over what is
>> >> > > considered a good backing array size.
>> >> >
>> >> > Coleen raised a good question in a separate
discussion,
>> which made me
>> >> > realize that once the table has been initially
populated all
>> >> subsequent
>> >> > additions, and hence all subsequent calls to grow()
always
>> happen
>> >> with
>> >> > the Threads_lock held. So we can't just defer the
grow().
>> >> >
>> >> > >> That aside, I'd like to know how expensive it is
to
>> grow this
>> >> table.
>> >> > >> What are we talking about here?
>> >> > >
>> >> > > We use global counter which on write_synchronize
must
>> scan all
>> >> > > threads to make sure they have seen the update
(there some
>> >> > > optimization to avoid it if there is no readers
at all).
>> Since this
>> >> > > table contains the threads, we get double
penalized, for
>> each new
>> >> > > thread the synchronization cost increase AND the
number
>> of items.
>> >> > >
>> >> > > With concurrent reads you still need many
thousands of
>> threads, but
>> >> > > I think I saw someone mentioning 100k threads,
assuming
>> concurrent
>> >> > > queries the resize can take hundreds of ms to
finish.
>> Note that
>> >> reads
>> >> > > and inserts still in operate roughly at the same
speed
>> while
>> >> > > resizing. So a longer resize is only problematic
if we
>> do not
>> >> > > respect safepoints.
>> >> > I think if anything were capable of running 100K
threads
>> we would be
>> >> > hitting far worse scalability bottlenecks than
this. But
>> this does
>> >> seem
>> >> > problematic.
>> >> >
>> >> > Thanks,
>> >> > David
>> >> > -----
>> >> >
>> >> > > Thanks, Robbin
>> >> > >
>> >> > >>
>> >> > >> David
>> >> > >>
>> >> > >>> /Robbin
>> >> > >>>
>> >> > >>> On 2019-10-02 08:46, David Holmes wrote:
>> >> > >>>> Hi Daniil,
>> >> > >>>>
>> >> > >>>> On 2/10/2019 4:13 pm, Daniil Titov wrote:
>> >> > >>>>> Please review a change that fixes the issue.
The
>> problem
>> >> here is
>> >> > >>>>> that that the thread is added to the
ThreadIdTable
>> >> (introduced in
>> >> > >>>>> [3]) while the Threads_lock is held by
>> >> > >>>>> JVM_StartThread. When new thread is added to
the
>> thread
>> >> table the
>> >> > >>>>> table checks if its load factor is greater
than
>> required and
>> >> if so
>> >> > >>>>> it grows itself while polling for safepoints.
>> >> > >>>>> After changes [4] an attempt to block the
thread while
>> >> holding the
>> >> > >>>>> Threads_lock results in assertion in
>> >> > >>>>> Thread::check_possible_safepoint().
>> >> > >>>>>
>> >> > >>>>> The fix proposed by David Holmes ( thank you,
>> David!) is
>> >> to skip
>> >> > >>>>> the ThreadBlockInVM inside
ThreadIdTable::grow()
>> method if the
>> >> > >>>>> current thread owns the Threads_lock.
>> >> > >>>>
>> >> > >>>> Sorry but looking at the fix in context now I
think
>> it would be
>> >> > >>>> better to do this:
>> >> > >>>>
>> >> > >>>> while (gt.do_task(jt)) {
>> >> > >>>> if (Threads_lock->owner() == jt) {
>> >> > >>>> gt.pause(jt);
>> >> > >>>> ThreadBlockInVM tbivm(jt);
>> >> > >>>> gt.cont(jt);
>> >> > >>>> }
>> >> > >>>> }
>> >> > >>>>
>> >> > >>>> This way we don't waste time with the
pause/cont when
>> there's no
>> >> > >>>> safepoint pause going to happen - and the
owner()
>> check is
>> >> quicker
>> >> > >>>> than owned_by_self(). That partially addresses
a general
>> >> concern I
>> >> > >>>> have about how long it may take to grow the
table, as
>> we are
>> >> > >>>> deferring safepoints until it is complete in
this
>> >> JVM_StartThread
>> >> > >>>> usecase.
>> >> > >>>>
>> >> > >>>> In the test you don't need all of:
>> >> > >>>>
>> >> > >>>> 32 * @run clean ThreadStartTest
>> >> > >>>> 33 * @run build ThreadStartTest
>> >> > >>>> 34 * @run main ThreadStartTest
>> >> > >>>>
>> >> > >>>> just the last @run suffices to build and run
the test.
>> >> > >>>>
>> >> > >>>> Thanks,
>> >> > >>>> David
>> >> > >>>> -----
>> >> > >>>>
>> >> > >>>>> Testing : Mach 5 tier1 and tier2 completed
>> successfully,
>> >> tier3 is
>> >> > >>>>> in progress.
>> >> > >>>>>
>> >> > >>>>> [1] Webrev:
>> >> http://cr.openjdk.java.net/~dtitov/8231666/webrev.01/
>> >> > >>>>> [2] Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8231666
>> >> > >>>>> [3]
https://bugs.openjdk.java.net/browse/JDK-8185005
>> >> > >>>>> [4]
https://bugs.openjdk.java.net/browse/JDK-8184732
>> >> > >>>>>
>> >> > >>>>> Best regards,
>> >> > >>>>> Danill
>> >> > >>>>>
>> >> > >>>>>
>> >> >
>> >> >
>> >> >
>> >>
>> >>
>>
>>
