On Tue, 17 Jan 2023 13:23:51 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
Marked as reviewed by dholmes (Revie
> test of tier1-5 passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
8292741: Convert JvmtiTagMapTable to ResourceHashtable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11288/files
- new: https://git.open
> test of tier1-5 passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
8292741: Convert JvmtiTagMapTable to ResourceHashtable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11288/files
- new: https://git.open
On Wed, 11 Jan 2023 10:01:57 GMT, Afshin Zafari wrote:
>> The JvmtiTagMap code adds/updates and removes the entries like below in two
>> places which could probably be simplified, but I think that's outside the
>> scope of this RFE. I suggest removing the bool return from add, remove and
>>
On Mon, 12 Dec 2022 00:44:29 GMT, David Holmes wrote:
>> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 88:
>>
>>> 86: }
>>> 87:
>>> 88: JvmtiTagMapTable::~JvmtiTagMapTable() {
>>
>> Is this the only use of `clear`? If so we can just inline its code here and
>> remove it from the API.
>
>
On Thu, 15 Dec 2022 01:53:32 GMT, David Holmes wrote:
>> In the `iterate` method of ResourceHashTable, in respurceHash.hpp lines
>> 227-240 copied here, requires a function that returns boolean. If returned
>> false the iterate will break.
>>
>> template
>> void iterate(Function function) c
On Wed, 11 Jan 2023 10:49:59 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
Still have a couple of nits regardin
On Wed, 11 Jan 2023 10:49:59 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
I have no further comments. This pa
> test of tier1-5 passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
8292741: Convert JvmtiTagMapTable to ResourceHashtable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11288/files
- new: https://git.open
> test of tier1-5 passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
8292741: Convert JvmtiTagMapTable to ResourceHashtable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11288/files
- new: https://git.open
On Fri, 6 Jan 2023 14:13:05 GMT, Coleen Phillimore wrote:
>> The issue is not the underlying RHT methods but the semantics of the
>> `jvmtiTagMap` methods. If a call to add always expects to add then it should
>> be a fatal error if it actually did an update as that indicates something is
>> b
On Tue, 10 Jan 2023 23:36:37 GMT, Coleen Phillimore wrote:
>> Afshin Zafari has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - 8292741: Convert JvmtiTagMapTable to ResourceHashtable
>> - 8292741: Convert JvmtiTagMapTable to ResourceHas
On Tue, 10 Jan 2023 16:56:31 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - 8292741: Convert JvmtiTagMapTable to ResourceHashtable
> - 8292741: Convert JvmtiTagMap
On Tue, 10 Jan 2023 16:56:31 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - 8292741: Convert JvmtiTagMapTable to ResourceHashtable
> - 8292741: Convert JvmtiTagMap
> test of tier1-5 passed.
Afshin Zafari has updated the pull request incrementally with three additional
commits since the last revision:
- 8292741: Convert JvmtiTagMapTable to ResourceHashtable
- 8292741: Convert JvmtiTagMapTable to ResourceHashtable
- 8292741: Convert JvmtiTagMapTable to Re
On Thu, 15 Dec 2022 01:57:27 GMT, David Holmes wrote:
>> I agree that this is ambigious. The `jvmtiTagMap` calls `add/update` methods
>> of `jvmtiTagMapTable` which in turn calls `resourceHashTable` methods `put`
>> and `put_if_absent`.
>> `put` and `put_if_absent` can be used for both adding a
On Thu, 8 Dec 2022 16:56:40 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
Looks fine, thanks. (+- DavidH commen
On Wed, 14 Dec 2022 15:11:55 GMT, Afshin Zafari wrote:
>> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 114:
>>
>>> 112: JvmtiTagMapKey new_entry(obj);
>>> 113: bool is_updated = _table.put(new_entry, tag) == false;
>>> 114: assert(is_updated, "should be updated and not added");
>>
>>
On Wed, 14 Dec 2022 13:28:02 GMT, Afshin Zafari wrote:
>> src/hotspot/share/prims/jvmtiTagMap.cpp line 1262:
>>
>>> 1260: // and record the reference and tag value.
>>> 1261: //
>>> 1262: bool do_entry(JvmtiTagMapKey& key, jlong& value) {
>>
>> I still see no point in this method returnin
On Mon, 12 Dec 2022 00:50:27 GMT, David Holmes wrote:
>> Afshin Zafari has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
>
> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 114:
>
>
On Mon, 12 Dec 2022 00:40:14 GMT, David Holmes wrote:
>> Afshin Zafari has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 1262:
>
>> 12
On Mon, 12 Dec 2022 17:58:53 GMT, Coleen Phillimore wrote:
>> Afshin Zafari has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
>
> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 117:
On Thu, 8 Dec 2022 16:56:40 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
The alternative to force-push would h
On Thu, 8 Dec 2022 16:56:40 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
I have a couple of minor comments but
On Mon, 12 Dec 2022 14:44:03 GMT, Robbin Ehn wrote:
> > Also please do not force-push.
>
> FYI: I'm not sure it even was possible to fix the "full of unexpected
> changes" without force push. Even if it was, the effort required to fix it
> without force push would be pretty significant. So I'd
On Mon, 12 Dec 2022 00:58:35 GMT, David Holmes wrote:
> Also please do not force-push.
FYI: I'm not sure it even was possible to fix the "full of unexpected changes"
without force push.
Even if it was, the effort required to fix it without force push would be
pretty significant.
So I'd say thi
On Mon, 28 Nov 2022 01:23:10 GMT, David Holmes wrote:
>> Afshin Zafari has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
>
> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 88:
>
>>
On Mon, 28 Nov 2022 22:17:14 GMT, David Holmes wrote:
>> ResourceHashTable::put() returns true if the Key,Value is added, false if
>> the Value is updated.
>
> But this doesn't do that, so ??
This issue is not resolved.
-
PR: https://git.openjdk.org/jdk/pull/11288
On Thu, 8 Dec 2022 16:56:40 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8292741: Convert JvmtiTagMapTable to ResourceHashtable
I'm still seeing a few oddities in th
On Tue, 6 Dec 2022 04:54:50 GMT, David Holmes wrote:
> This PR seems to be broken now - full of unexpected changes.
The PR changed files are correct now.
-
PR: https://git.openjdk.org/jdk/pull/11288
> test of tier1-5 passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
8292741: Convert JvmtiTagMapTable to ResourceHashtable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11288/files
- new: https://git.open
> test of tier1-5 passed.
Afshin Zafari has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains six additional
commits since the last revision:
- 8292741: Co
> test of tier1-5 passed.
Afshin Zafari has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains four commits:
- 8292741: Convert JvmtiTagMapTable to ResourceHashtable
- 8292741: Convert JvmtiTagMapTable to ResourceHashtable
- Added update
> test of tier1-5 passed.
Afshin Zafari has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 10 commits:
- Merge branch 'jvmti_tagmap' of http://github.com/afshin-zafari/jdk into
jvmti_tagmap
- 8292741: Convert JvmtiTagMapTable to Resou
> test of tier1-5 passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
8292741: Convert JvmtiTagMapTable to ResourceHashtable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11288/files
- new: https://git.open
On Mon, 5 Dec 2022 13:16:44 GMT, Afshin Zafari wrote:
>> test of tier1-5 passed.
>
> Afshin Zafari has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains
> test of tier1-5 passed.
Afshin Zafari has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains seven additional
commits since the last revision:
- 8292741:
On Tue, 22 Nov 2022 14:48:11 GMT, Afshin Zafari wrote:
> test of tier1-5 passed.
src/hotspot/share/prims/jvmtiTagMapTable.hpp line 41:
> 39: JvmtiTagMapEntry* next() const {
> 40: return (JvmtiTagMapEntry*)HashtableEntry mtServiceability>::next();
> 41: }
This should add a comment abov
On Tue, 29 Nov 2022 23:07:15 GMT, Coleen Phillimore wrote:
>> Are you suggesting that `add` can also act as a `replace` operation? I
>> would think we would want a separate method for that.
>
> Yes, this could have a replace operation that does below which would be
> better:
>
> void JvmtiTag
On Mon, 28 Nov 2022 22:14:06 GMT, David Holmes wrote:
>> add() is supposed to add a new object with tag. There is an assert() in its
>> body that checks it. By removing the assert, add() can be used for updating
>> as well.
>
> Are you suggesting that `add` can also act as a `replace` operatio
On Tue, 29 Nov 2022 12:22:38 GMT, Afshin Zafari wrote:
>> Not sure ... didn't we start using C++ lambda's for some of these "closure"
>> operations? @coleenp what is the usual pattern we use for this kind of thing?
>
> The `unlink` method of `ResourceHashTable` gets an ITER type and calls its
>
On Mon, 28 Nov 2022 22:16:34 GMT, David Holmes wrote:
>> Coleen's suggestion for efficiency reasons.
>
> If the comment is meant to suggest some possible future optimisation it
> should say that.
Oh you should uncomment that code. The hashtable that this replaces has the
same early cut-out for
On Mon, 28 Nov 2022 22:12:22 GMT, David Holmes wrote:
>> Alternative for struct?
>
> Not sure ... didn't we start using C++ lambda's for some of these "closure"
> operations? @coleenp what is the usual pattern we use for this kind of thing?
The `unlink` method of `ResourceHashTable` gets an ITE
On Mon, 28 Nov 2022 11:35:48 GMT, Afshin Zafari wrote:
>> src/hotspot/share/prims/jvmtiTagMap.cpp line 254:
>>
>>> 252: if (obj_tag != current_tag ) {
>>> 253: hashmap->remove(o);
>>> 254: hashmap->add(o, obj_tag);
>>
>> This change is not atomic - is that a problem? The c
On Mon, 28 Nov 2022 01:23:30 GMT, David Holmes wrote:
>> test of tier1-5 passed.
>
> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 95:
>
>> 93://if (obj->fast_no_hash_check()) {
>> 94:// return 0;
>> 95://} else {
>
> What are these comments?
Coleen's suggestion for efficiency
On Mon, 28 Nov 2022 01:20:49 GMT, David Holmes wrote:
>> test of tier1-5 passed.
>
> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 77:
>
>> 75:
>> 76: void JvmtiTagMapTable::clear() {
>> 77: struct RemoveAll{
>
> Nit: space before {
>
> Stylistically I'm not sure we define local structs
On Mon, 28 Nov 2022 01:12:18 GMT, David Holmes wrote:
>> test of tier1-5 passed.
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 254:
>
>> 252: if (obj_tag != current_tag ) {
>> 253: hashmap->remove(o);
>> 254: hashmap->add(o, obj_tag);
>
> This change is not atomic - is
On Tue, 22 Nov 2022 14:48:11 GMT, Afshin Zafari wrote:
> test of tier1-5 passed.
Hi Afshin,
The general conversion approach seems okay, though it is hard to see exactly
how the new table is used compared to the old table i.e. in relation to the
role of `JvmtiTagMapEntry` now that it is not n
test of tier1-5 passed.
-
Commit messages:
- JBS-8292741: Convert JvmtiTagMapTable to ResourceHashtable
Changes: https://git.openjdk.org/jdk/pull/11288/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11288&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8292741
Stats
49 matches
Mail list logo
