You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by "theigl (via GitHub)" <gi...@apache.org> on 2023/04/10 15:22:19 UTC
[GitHub] [wicket] theigl opened a new pull request, #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
theigl opened a new pull request, #572:
URL: https://github.com/apache/wicket/pull/572
This PR avoids unnecessary allocation of `AppendingStringBuffer` for a `StringResponse` that is never written to.
In our case, 99% of the allocations in `Component#internalRenderHead` can be avoided in this way.
I'm not sure if we should add `LazyStringResponse` as a separate class, or simply make the existing `StringResponse` lazy. It is used in a lot of places, so we might avoid more allocations if we changed the default implementation.
See https://issues.apache.org/jira/browse/WICKET-7041
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a diff in pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #572:
URL: https://github.com/apache/wicket/pull/572#discussion_r1161997017
##########
wicket-core/src/main/java/org/apache/wicket/response/LazyStringResponse.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.response;
+
+import org.apache.wicket.request.Response;
+import org.apache.wicket.util.string.AppendingStringBuffer;
+
+
+/**
+ * Response object that writes to a StringWriter. If the StringResponse is later converted to a
+ * String via toString(), the output which was written to the StringResponse will be returned as a
+ * String.
+ *
+ * @author Thomas Heigl
+ */
+public class LazyStringResponse extends Response
+{
+
+ /** StringWriter to write to */
+ private AppendingStringBuffer out;
+
+ /**
+ * @see Response#write(CharSequence)
+ */
+ @Override
+ public void write(final CharSequence string)
+ {
+ if (out == null)
+ {
+ out = new AppendingStringBuffer(128);
Review Comment:
`128` should be a constructor parameter. The default constructor should pass `128` to another ctor that accepts the initial size
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "bitstorm (via GitHub)" <gi...@apache.org>.
bitstorm commented on PR #572:
URL: https://github.com/apache/wicket/pull/572#issuecomment-1503587570
> > @chrisco484 I believe this one will definitely be merged to 9.x
>
> @solomax Any earlier versions like 7 or 8 ? (Optimistic I know ;) )
If it's just (as it seems...) a cherry pick we might port this back to Wicket 8.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] chrisco484 commented on pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "chrisco484 (via GitHub)" <gi...@apache.org>.
chrisco484 commented on PR #572:
URL: https://github.com/apache/wicket/pull/572#issuecomment-1502716053
Any chance this might be backported to earlier releases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a diff in pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "theigl (via GitHub)" <gi...@apache.org>.
theigl commented on code in PR #572:
URL: https://github.com/apache/wicket/pull/572#discussion_r1162053099
##########
wicket-core/src/main/java/org/apache/wicket/response/LazyStringResponse.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.response;
+
+import org.apache.wicket.request.Response;
+import org.apache.wicket.util.string.AppendingStringBuffer;
+
+
+/**
+ * Response object that writes to a StringWriter. If the StringResponse is later converted to a
+ * String via toString(), the output which was written to the StringResponse will be returned as a
+ * String.
+ *
+ * @author Thomas Heigl
+ */
+public class LazyStringResponse extends Response
+{
+
+ /** StringWriter to write to */
+ private AppendingStringBuffer out;
+
+ /**
+ * @see Response#write(CharSequence)
+ */
+ @Override
+ public void write(final CharSequence string)
+ {
+ if (out == null)
+ {
+ out = new AppendingStringBuffer(128);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a diff in pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #572:
URL: https://github.com/apache/wicket/pull/572#discussion_r1161998177
##########
wicket-core/src/main/java/org/apache/wicket/response/LazyStringResponse.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.response;
+
+import org.apache.wicket.request.Response;
+import org.apache.wicket.util.string.AppendingStringBuffer;
+
+
+/**
+ * Response object that writes to a StringWriter. If the StringResponse is later converted to a
+ * String via toString(), the output which was written to the StringResponse will be returned as a
+ * String.
Review Comment:
Copy/pasted javadoc from StringResponse.
Please add something about its uniqueness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] chrisco484 commented on pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "chrisco484 (via GitHub)" <gi...@apache.org>.
chrisco484 commented on PR #572:
URL: https://github.com/apache/wicket/pull/572#issuecomment-1503022859
> @chrisco484 I believe this one will definitely be merged to 9.x
Any earlier versions like 7 or 8 ? (Optimistic I know ;) )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl merged pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "theigl (via GitHub)" <gi...@apache.org>.
theigl merged PR #572:
URL: https://github.com/apache/wicket/pull/572
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a diff in pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "solomax (via GitHub)" <gi...@apache.org>.
solomax commented on code in PR #572:
URL: https://github.com/apache/wicket/pull/572#discussion_r1161881637
##########
wicket-core/src/main/java/org/apache/wicket/response/LazyStringResponse.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.response;
+
+import org.apache.wicket.request.Response;
+import org.apache.wicket.util.string.AppendingStringBuffer;
+
+
+/**
+ * Response object that writes to a StringWriter. If the StringResponse is later converted to a
+ * String via toString(), the output which was written to the StringResponse will be returned as a
+ * String.
+ *
+ * @author Thomas Heigl
+ */
+public class LazyStringResponse extends Response
+{
+
+ /** StringWriter to write to */
+ private AppendingStringBuffer out;
+
+ /**
+ * @see Response#write(CharSequence)
+ */
+ @Override
+ public void write(final CharSequence string)
+ {
+ getOut().append(string);
+ }
+
+ /**
+ * @see Response#reset()
+ */
+ @Override
+ public void reset()
+ {
+ getOut().clear();
Review Comment:
Is it necessary to create buffer if you only need to reset it?
Maybe this code can be changed to:
`if (out != null) out.reset() ?`
If so `getOut()` method can be dropped :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a diff in pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "theigl (via GitHub)" <gi...@apache.org>.
theigl commented on code in PR #572:
URL: https://github.com/apache/wicket/pull/572#discussion_r1161885681
##########
wicket-core/src/main/java/org/apache/wicket/response/LazyStringResponse.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.response;
+
+import org.apache.wicket.request.Response;
+import org.apache.wicket.util.string.AppendingStringBuffer;
+
+
+/**
+ * Response object that writes to a StringWriter. If the StringResponse is later converted to a
+ * String via toString(), the output which was written to the StringResponse will be returned as a
+ * String.
+ *
+ * @author Thomas Heigl
+ */
+public class LazyStringResponse extends Response
+{
+
+ /** StringWriter to write to */
+ private AppendingStringBuffer out;
+
+ /**
+ * @see Response#write(CharSequence)
+ */
+ @Override
+ public void write(final CharSequence string)
+ {
+ getOut().append(string);
+ }
+
+ /**
+ * @see Response#reset()
+ */
+ @Override
+ public void reset()
+ {
+ getOut().clear();
Review Comment:
Ha, i just realized the same thing and pushed it before reading your comment ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] solomax commented on pull request #572: WICKET-7041 Introduce `LazyStringResponse` to avoid unnecessary allocations
Posted by "solomax (via GitHub)" <gi...@apache.org>.
solomax commented on PR #572:
URL: https://github.com/apache/wicket/pull/572#issuecomment-1502724872
@chrisco484 I believe this one will definitely be merged to 9.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org