RFR (S): 8142749: HeapRegion cleanup

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (S): 8142749: HeapRegion cleanup

Всеволод Толстопятов
Hi all,
Please review and sponsor this change for JDK-8142749.
Note that not only HeapRegion::_predicted_bytes_to_copy related code was removed, but also some unused (since the first commit) method/class parameters and HeapRegion::object_iterate_mem_careful method. All changed files contain at least several unused includes, but I'm not sure it's applicable to remove them in this patch.

CR: https://bugs.openjdk.java.net/browse/JDK-8142749
Testing: hotspot_gc tests

--
Best regards,
Tolstopyatov Vsevolod
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S): 8142749: HeapRegion cleanup

Thomas Schatzl
Hi,

On Mon, 2016-05-30 at 15:05 +0300, Всеволод Толстопятов wrote:

> Hi all,
> Please review and sponsor this change for JDK-8142749.
> Note that not only HeapRegion::_predicted_bytes_to_copy related code
> was removed, but also some unused (since the first commit)
> method/class parameters and HeapRegion::object_iterate_mem_careful
> method. All changed files contain at least several unused includes,
> but I'm not sure it's applicable to remove them in this patch.
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8142749
> Webrev: http://cr.openjdk.java.net/~fzhinkin/vtolstopyatov/8142749/we
> brev.00/
> Testing: hotspot_gc tests

  the change looks good, and I can sponsor it.

I see no reason to not remove the superfluous includes of the files you
are touching in this change.

Note that at the moment are past the JDK9 FC date, and we should not
integrate enhancements right now. There is no process at the moment if
and how these can be integrated. So it may take a while to push the
change.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S): 8142749: HeapRegion cleanup

Derek White
In reply to this post by Всеволод Толстопятов
On 5/30/16 8:05 AM, Всеволод Толстопятов wrote:
Hi all,
Please review and sponsor this change for JDK-8142749.
Note that not only HeapRegion::_predicted_bytes_to_copy related code was removed, but also some unused (since the first commit) method/class parameters and HeapRegion::object_iterate_mem_careful method. All changed files contain at least several unused includes, but I'm not sure it's applicable to remove them in this patch.

CR: https://bugs.openjdk.java.net/browse/JDK-8142749
Testing: hotspot_gc tests

--
Best regards,
Tolstopyatov Vsevolod

Looks good!


 - Derek

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S): 8142749: HeapRegion cleanup

Всеволод Толстопятов
Thanks for review and sponsoring!
 
Note that at the moment are past the JDK9 FC date, and we should not
integrate enhancements right now. There is no process at the moment if
and how these can be integrated. So it may take a while to push the
change.

Should I wait until JDK10 forest and ping someone? Should I update webrev with includes cleanup now or wait until new forest? 
Does it make sense to keep making reviews for simple bugs (e.g. like JDK-8151045) or it's better to wait until new forest (note that I'm not openjdk author yet, so these questions could have obvious answers)? 


--
Best regards,
Tolstopyatov Vsevolod

On Wed, Jun 8, 2016 at 12:51 AM, Derek White <[hidden email]> wrote:
On 5/30/16 8:05 AM, Всеволод Толстопятов wrote:
Hi all,
Please review and sponsor this change for JDK-8142749.
Note that not only HeapRegion::_predicted_bytes_to_copy related code was removed, but also some unused (since the first commit) method/class parameters and HeapRegion::object_iterate_mem_careful method. All changed files contain at least several unused includes, but I'm not sure it's applicable to remove them in this patch.

CR: https://bugs.openjdk.java.net/browse/JDK-8142749
Testing: hotspot_gc tests

--
Best regards,
Tolstopyatov Vsevolod

Looks good!


 - Derek


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S): 8142749: HeapRegion cleanup

Thomas Schatzl
Hi,

  sorry for the somewhat later answer...

On Thu, 2016-06-09 at 11:26 +0300, Всеволод Толстопятов wrote:
> Thanks for review and sponsoring!
>  
> > Note that at the moment are past the JDK9 FC date, and we should
> > not integrate enhancements right now. There is no process at the
> > moment if and how these can be integrated. So it may take a while
> > to push the change.
> Should I wait until JDK10 forest and ping someone?

I would appreciate if you pinged me again as soon as there is a jdk10
tree.

> Should I update webrev with includes cleanup now or wait until new
> forest? 

I think the include cleanups could be added.

> Does it make sense to keep making reviews for simple bugs (e.g. like
> JDK-8151045) or it's better to wait until new forest (note that I'm
> not openjdk author yet, so these questions could have obvious
> answers)? 

I do not know how posting reviews for simple bugs would be different
before or after.

The only difference is one of convenience: if you have two commits
contributed by you, you can apply for authorship, which makes posting
contributions easier.

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S): 8142749: HeapRegion cleanup

Thomas Schatzl
Hi,

On Tue, 2016-06-14 at 14:16 +0200, Thomas Schatzl wrote:

> Hi,
>
>   sorry for the somewhat later answer...
>
> On Thu, 2016-06-09 at 11:26 +0300, Всеволод Толстопятов wrote:
> >
> > Thanks for review and sponsoring!
> >  
> > >
> > > Note that at the moment are past the JDK9 FC date, and we should
> > > not integrate enhancements right now. There is no process at the
> > > moment if and how these can be integrated. So it may take a while
> > > to push the change.
> > Should I wait until JDK10 forest and ping someone? 

  pushing now. Thanks again for contributing, and very sorry for the
very long delay.

Note that in the meantime somebody else removed
HeapRegion::object_iterate_mem_careful(). I ignored the related hunks
in the push.

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S): 8142749: HeapRegion cleanup

Всеволод Толстопятов
Wow, I've almost forgot about this one.
Thanks!
Does it mean (since jdk10 forest was created) I can try to contribute more [small] patches and they will be processed a little bit faster? :)

--
Best regards,
Tolstopyatov Vsevolod

On Tue, Feb 7, 2017 at 2:59 PM, Thomas Schatzl <[hidden email]> wrote:
Hi,

On Tue, 2016-06-14 at 14:16 +0200, Thomas Schatzl wrote:
> Hi,
>
>   sorry for the somewhat later answer...
>
> On Thu, 2016-06-09 at 11:26 +0300, Всеволод Толстопятов wrote:
> >
> > Thanks for review and sponsoring!
> >  
> > >
> > > Note that at the moment are past the JDK9 FC date, and we should
> > > not integrate enhancements right now. There is no process at the
> > > moment if and how these can be integrated. So it may take a while
> > > to push the change.
> > Should I wait until JDK10 forest and ping someone? 

  pushing now. Thanks again for contributing, and very sorry for the
very long delay.

Note that in the meantime somebody else removed
HeapRegion::object_iterate_mem_careful(). I ignored the related hunks
in the push.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S): 8142749: HeapRegion cleanup

Thomas Schatzl
Hi,

On Wed, 2017-02-08 at 21:28 +0100, Vsevolod Tolstopyatov wrote:
> Wow, I've almost forgot about this one.
> Thanks!
> Does it mean (since jdk10 forest was created) I can try to contribute
> more [small] patches and they will be processed a little bit faster?
> :)

Yes :)

Thomas

Loading...