You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by GitBox <gi...@apache.org> on 2020/05/29 15:59:32 UTC
[GitHub] [wicket] theigl opened a new pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
theigl opened a new pull request #437:
URL: https://github.com/apache/wicket/pull/437
This PR avoids splitting and joining ajax event names that do not contain whitespace.
Note: The splitting logic splits on "\s+" so this would also match tabs and newlines, but I think that checking for space is enough to return early.
See https://issues.apache.org/jira/browse/WICKET-6795
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433122027
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
We should either use `' '` as a split char everywhere or `\s+`.
At the moment it is inconsistent and IMO not very good.
What about:
```
diff --git wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
index a03f8d2a42..de6ed89a56 100644
--- wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
+++ wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
@@ -18,6 +18,8 @@ package org.apache.wicket.ajax;
import java.util.ArrayList;
import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import org.apache.wicket.Component;
import org.apache.wicket.ajax.attributes.AjaxRequestAttributes;
@@ -68,6 +70,11 @@ public abstract class AjaxEventBehavior extends AbstractDefaultAjaxBehavior
private static final long serialVersionUID = 1L;
+ /**
+ * Splits event
+ */
+ private static final Pattern EVENT_NAME_SPLITTER = Pattern.compile("\\s+");
+
private final String event;
/**
@@ -119,12 +126,13 @@ public abstract class AjaxEventBehavior extends AbstractDefaultAjaxBehavior
*/
public String getEvent()
{
- if (event.indexOf(' ') == -1)
+ final Matcher matcher = EVENT_NAME_SPLITTER.matcher(event);
+ if (!matcher.matches())
{
return event;
}
- String[] splitEvents = event.split("\\s+");
+ String[] splitEvents = EVENT_NAME_SPLITTER.split(event);
List<String> cleanedEvents = new ArrayList<>(splitEvents.length);
for (String evt : splitEvents)
{
```
?
How much slower is it than using `' '` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433374344
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
Thanks @martin-g!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433166508
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
I will extract a constant for the separator and use it in both places.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433162126
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
I don't mind to use `' '` in both places for 9.x.
I also always thought that the separator is just space.
If someone complains and gives us a good reason then we can reconsider.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433136846
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
@martin-g: My working assumption was that `\\s+` represents multiple space characters and was not actually intended for for matching tabs and newlines. But you are right, it doesn't look too great. I'll write a quick benchmark to compare performance of both solutions: Alternatively, we could simply look for space in both cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433122027
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
We should either use `' '` as a split char everywhere or `\s+`.
At the moment it is inconsistent and IMO not very good.
What about:
```diff
diff --git wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
index a03f8d2a42..de6ed89a56 100644
--- wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
+++ wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
@@ -18,6 +18,8 @@ package org.apache.wicket.ajax;
import java.util.ArrayList;
import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import org.apache.wicket.Component;
import org.apache.wicket.ajax.attributes.AjaxRequestAttributes;
@@ -68,6 +70,11 @@ public abstract class AjaxEventBehavior extends AbstractDefaultAjaxBehavior
private static final long serialVersionUID = 1L;
+ /**
+ * Splits event
+ */
+ private static final Pattern EVENT_NAME_SPLITTER = Pattern.compile("\\s+");
+
private final String event;
/**
@@ -119,12 +126,13 @@ public abstract class AjaxEventBehavior extends AbstractDefaultAjaxBehavior
*/
public String getEvent()
{
- if (event.indexOf(' ') == -1)
+ final Matcher matcher = EVENT_NAME_SPLITTER.matcher(event);
+ if (!matcher.matches())
{
return event;
}
- String[] splitEvents = event.split("\\s+");
+ String[] splitEvents = EVENT_NAME_SPLITTER.split(event);
List<String> cleanedEvents = new ArrayList<>(splitEvents.length);
for (String evt : splitEvents)
{
```
?
How much slower is it than using `' '` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433144531
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
@martin-g: `indexOf` is nearly 10 times faster than using `Matcher`.
```
Benchmark Mode Cnt Score Error Units
AjaxEventBenchmarks.getEventIndexOf thrpt 5 193156786,374 ± 4206548,589 ops/s
AjaxEventBenchmarks.getEventMatcher thrpt 5 21352485,044 ± 3780313,472 ops/s
AjaxEventBenchmarks.getEventOriginal thrpt 5 6107376,960 ± 216974,067 ops/s
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433322930
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
https://issues.apache.org/jira/browse/WICKET-6797
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #437:
URL: https://github.com/apache/wicket/pull/437#discussion_r433122027
##########
File path: wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
##########
@@ -119,11 +119,16 @@ protected void updateAjaxAttributes(AjaxRequestAttributes attributes)
*/
public String getEvent()
{
+ if (event.indexOf(' ') == -1)
Review comment:
We should either use `' '` as a split char everywhere or `\s+`.
At the moment it is inconsistent and IMO not very good.
What about:
```diff
diff --git wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
index a03f8d2a42..de6ed89a56 100644
--- wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
+++ wicket-core/src/main/java/org/apache/wicket/ajax/AjaxEventBehavior.java
@@ -18,6 +18,8 @@ package org.apache.wicket.ajax;
import java.util.ArrayList;
import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import org.apache.wicket.Component;
import org.apache.wicket.ajax.attributes.AjaxRequestAttributes;
@@ -68,6 +70,11 @@ public abstract class AjaxEventBehavior extends AbstractDefaultAjaxBehavior
private static final long serialVersionUID = 1L;
+ private static final Pattern EVENT_NAME_SPLITTER = Pattern.compile("\\s+");
+
private final String event;
/**
@@ -119,12 +126,13 @@ public abstract class AjaxEventBehavior extends AbstractDefaultAjaxBehavior
*/
public String getEvent()
{
- if (event.indexOf(' ') == -1)
+ final Matcher matcher = EVENT_NAME_SPLITTER.matcher(event);
+ if (!matcher.matches())
{
return event;
}
- String[] splitEvents = event.split("\\s+");
+ String[] splitEvents = EVENT_NAME_SPLITTER.split(event);
List<String> cleanedEvents = new ArrayList<>(splitEvents.length);
for (String evt : splitEvents)
{
```
?
How much slower is it than using `' '` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl merged pull request #437: WICKET-6795 Avoid splitting and joining ajax event names
Posted by GitBox <gi...@apache.org>.
theigl merged pull request #437:
URL: https://github.com/apache/wicket/pull/437
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org