Skip to content

Improve performance for const schemas #511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Aug 29, 2022
Merged

Improve performance for const schemas #511

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
68ff81c
Improved performance for const schema
DanieleFedeli Aug 16, 2022
dbd3395
Merge branch 'master' of github.com:DanieleFedeli/fast-json-stringify
DanieleFedeli Aug 16, 2022
5fa6e25
Improve const object schema performance
DanieleFedeli Aug 16, 2022
ca55edf
Improve string and object performance
DanieleFedeli Aug 16, 2022
b2ce4d5
Remove unwanted validation for 'const' properties
DanieleFedeli Aug 25, 2022
05c92cf
Restored example file
DanieleFedeli Aug 25, 2022
f73da5a
Changed findIndex with indexOf
DanieleFedeli Aug 25, 2022
e2c3bd6
Removed useless inference when schema is const
DanieleFedeli Aug 25, 2022
ca1357d
Review requests
DanieleFedeli Aug 26, 2022
844eba5
Rebase to #510 (use hasOwnProperty from Object.prototype)
DanieleFedeli Aug 26, 2022
49301dc
Use isRequired when available
DanieleFedeli Aug 26, 2022
0eb362a
Restored FJS format style
DanieleFedeli Aug 27, 2022
b80bc4a
Restore FJS format style
DanieleFedeli Aug 27, 2022
3879db9
Added support for nullable / type: [..., 'null']
DanieleFedeli Aug 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 56 additions & 2 deletions benchmark/bench.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,60 @@ const benchmarks = [
n5: 42,
b5: true
}
},
{
name: 'object with const string property',
schema: {
type: 'object',
properties: {
a: { const: 'const string' }
}
},
input: { a: 'const string' }
},
{
name: 'object with const number property',
schema: {
type: 'object',
properties: {
a: { const: 1 }
}
},
input: { a: 1 }
},
{
name: 'object with const bool property',
schema: {
type: 'object',
properties: {
a: { const: true }
}
},
input: { a: true }
},
{
name: 'object with const object property',
schema: {
type: 'object',
properties: {
foo: { const: { bar: 'baz' } }
}
},
input: {
foo: { bar: 'baz' }
}
},
{
name: 'object with const null property',
schema: {
type: 'object',
properties: {
foo: { const: null }
}
},
input: {
foo: null
}
}
]

Expand All @@ -222,10 +276,10 @@ async function runBenchmark (benchmark) {
return new Promise((resolve, reject) => {
let result = null
worker.on('error', reject)
worker.on('message', (benchResult) => {
worker.on('message', benchResult => {
result = benchResult
})
worker.on('exit', (code) => {
worker.on('exit', code => {
if (code === 0) {
resolve(result)
} else {
Expand Down
34 changes: 18 additions & 16 deletions example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,19 +61,21 @@ const stringify = fastJson({
}
})

console.log(stringify({
firstName: 'Matteo',
lastName: 'Collina',
age: 32,
now: new Date(),
reg: /"([^"]|\\")*"/,
foo: 'hello',
numfoo: 42,
test: 42,
strtest: '23',
arr: [{ str: 'stark' }, { str: 'lannister' }],
obj: { bool: true },
notmatch: 'valar morghulis',
notmatchobj: { a: true },
notmatchnum: 42
}))
console.log(
stringify({
firstName: 'Matteo',
lastName: 'Collina',
age: 32,
now: new Date(),
reg: /"([^"]|\\")*"/,
foo: 'hello',
numfoo: 42,
test: 42,
strtest: '23',
arr: [{ str: 'stark' }, { str: 'lannister' }],
obj: { bool: true },
notmatch: 'valar morghulis',
notmatchobj: { a: true },
notmatchnum: 42
})
)
110 changes: 73 additions & 37 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@ const buildAjv = require('./ajv')

let largeArraySize = 2e4
let largeArrayMechanism = 'default'
const validLargeArrayMechanisms = [
'default',
'json-stringify'
]
const validLargeArrayMechanisms = ['default', 'json-stringify']

const addComma = `
if (addComma) {
Expand All @@ -34,7 +31,9 @@ function isValidSchema (schema, name) {
name = ''
}
const first = validate.errors[0]
const err = new Error(`${name}schema is invalid: data${first.instancePath} ${first.message}`)
const err = new Error(
`${name}schema is invalid: data${first.instancePath} ${first.message}`
)
err.errors = isValidSchema.errors
throw err
}
Expand Down Expand Up @@ -122,7 +121,9 @@ function build (schema, options) {

if (options.rounding) {
if (!['floor', 'ceil', 'round'].includes(options.rounding)) {
throw new Error(`Unsupported integer rounding method ${options.rounding}`)
throw new Error(
`Unsupported integer rounding method ${options.rounding}`
)
}
}

Expand All @@ -138,7 +139,9 @@ function build (schema, options) {
if (!Number.isNaN(Number.parseInt(options.largeArraySize, 10))) {
largeArraySize = options.largeArraySize
} else {
throw new Error(`Unsupported large array size. Expected integer-like, got ${options.largeArraySize}`)
throw new Error(
`Unsupported large array size. Expected integer-like, got ${options.largeArraySize}`
)
}
}

Expand Down Expand Up @@ -205,11 +208,7 @@ const arrayKeywords = [
'contains'
]

const stringKeywords = [
'maxLength',
'minLength',
'pattern'
]
const stringKeywords = ['maxLength', 'minLength', 'pattern']

const numberKeywords = [
'multipleOf',
Expand Down Expand Up @@ -240,6 +239,7 @@ function inferTypeByKeyword (schema) {
for (var keyword of numberKeywords) {
if (keyword in schema) return 'number'
}

return schema.type
}

Expand All @@ -253,8 +253,11 @@ function addPatternProperties (location) {
if (properties[keys[i]]) continue
`

const patternPropertiesLocation = mergeLocation(location, 'patternProperties')
Object.keys(pp).forEach((regex) => {
const patternPropertiesLocation = mergeLocation(
location,
'patternProperties'
)
Object.keys(pp).forEach(regex => {
let ppLocation = mergeLocation(patternPropertiesLocation, regex)
if (pp[regex].$ref) {
ppLocation = resolveRef(ppLocation, pp[regex].$ref)
Expand All @@ -264,7 +267,11 @@ function addPatternProperties (location) {
try {
RegExp(regex)
} catch (err) {
throw new Error(`${err.message}. Found at ${regex} matching ${JSON.stringify(pp[regex])}`)
throw new Error(
`${err.message}. Found at ${regex} matching ${JSON.stringify(
pp[regex]
)}`
)
}

const valueCode = buildValue(ppLocation, 'obj[keys[i]]')
Expand Down Expand Up @@ -339,19 +346,21 @@ function buildCode (location) {
let code = ''

const propertiesLocation = mergeLocation(location, 'properties')
Object.keys(schema.properties || {}).forEach((key) => {
Object.keys(schema.properties || {}).forEach(key => {
let propertyLocation = mergeLocation(propertiesLocation, key)
if (schema.properties[key].$ref) {
propertyLocation = resolveRef(location, schema.properties[key].$ref)
schema.properties[key] = propertyLocation.schema
}

const sanitized = JSON.stringify(key)
const asString = JSON.stringify(sanitized)

// Using obj['key'] !== undefined instead of obj.hasOwnProperty(prop) for perf reasons,
// see https://github.com/mcollina/fast-json-stringify/pull/3 for discussion.

const sanitized = JSON.stringify(key)
const asString = JSON.stringify(sanitized)
const isRequired = schema.required !== undefined && schema.required.indexOf(key) !== -1
const isConst = schema.properties[key].const !== undefined

code += `
if (obj[${sanitized}] !== undefined) {
${addComma}
Expand All @@ -361,13 +370,24 @@ function buildCode (location) {
code += buildValue(propertyLocation, `obj[${JSON.stringify(key)}]`)

const defaultValue = schema.properties[key].default
const constValue = schema.properties[key].const

if (defaultValue !== undefined) {
code += `
} else {
${addComma}
json += ${asString} + ':' + ${JSON.stringify(JSON.stringify(defaultValue))}
json += ${asString} + ':' + ${JSON.stringify(
JSON.stringify(defaultValue)
)}
`
} else if (required.includes(key)) {
} else if (isRequired && isConst) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work in a different way.

  1. You should handle value serialization only in one place otherwise you will have code duplication and can make an error. In your case, you miss the null check here. So please handle all schema serialization logic inside buildValue function.
  2. We don't need to do this check for const

    fast-json-stringify/index.js

    Line 365 in 844eba5

    if (obj[${sanitized}] !== undefined) {
    .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null checks you mean this?

const schema = {
  "type": "object",
  "properties": {
    "foo": {
      "const": "bar"
    }
  }
}


stringify({ "foo": "bar" }) // { "foo": "bar" }
stringify({ "foo": "baz" }) // { "foo": "bar" }
stringify({ "foo": 1 }) // { "foo": "bar" }
stringify({}) // { }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer 2.
If I don't have that checks how can I handle this?

const schema = {
  "type": "object",
  "properties": {
    "foo": {
      "const": "bar"
    }
  }
}

stringify({}) // { }

Copy link
Member

@ivan-tymoshenko ivan-tymoshenko Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the const value shouldn't work as a default value. It shouldn't have any other additional behavior.

const schema = {
  "type": "object",
  "properties": {
    "foo": {
      "const": "bar"
    }
  },
  "required": ['foo']
}

stringify({}) // throw new Error('foo is required')

Copy link
Contributor Author

@DanieleFedeli DanieleFedeli Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am OK with that, but I remember that someone agrees on this comment.

Copy link
Member

@ivan-tymoshenko ivan-tymoshenko Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, it should. From my point of view. But let's wait for other opinions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are waiting for me, I'm leaving this to you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to hear @climba03003 opinion as it was his suggestion #505 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the schema said the property is required, and missing one from the input.
It just convenient to always put the value there since it is required.

Just like why we always put the const value regardless the input.

Copy link
Member

@climba03003 climba03003 Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not block if it turns to throw.
It consistences with the current required handling for other properties.

code += `
} else {
json += ${asString} + ':' + ${JSON.stringify(
JSON.stringify(constValue)
)}
`
} else if (isRequired) {
code += `
} else {
throw new Error('${sanitized} is required!')
Expand All @@ -380,7 +400,7 @@ function buildCode (location) {
})

for (const requiredProperty of required) {
if (schema.properties && schema.properties[requiredProperty] !== undefined) continue
if (schema.properties && schema.properties[requiredProperty] !== undefined) { continue }
code += `if (obj['${requiredProperty}'] === undefined) throw new Error('"${requiredProperty}" is required!')\n`
}

Expand Down Expand Up @@ -444,14 +464,20 @@ function mergeAllOfSchema (location, schema, mergedSchema) {
if (mergedSchema.additionalProperties === undefined) {
mergedSchema.additionalProperties = {}
}
Object.assign(mergedSchema.additionalProperties, allOfSchema.additionalProperties)
Object.assign(
mergedSchema.additionalProperties,
allOfSchema.additionalProperties
)
}

if (allOfSchema.patternProperties !== undefined) {
if (mergedSchema.patternProperties === undefined) {
mergedSchema.patternProperties = {}
}
Object.assign(mergedSchema.patternProperties, allOfSchema.patternProperties)
Object.assign(
mergedSchema.patternProperties,
allOfSchema.patternProperties
)
}

if (allOfSchema.required !== undefined) {
Expand Down Expand Up @@ -668,7 +694,9 @@ function buildArray (location) {
if (largeArrayMechanism === 'json-stringify') {
functionCode += `if (arrayLength && arrayLength >= ${largeArraySize}) return JSON.stringify(obj)\n`
} else {
throw new Error(`Unsupported large array mechanism ${largeArrayMechanism}`)
throw new Error(
`Unsupported large array mechanism ${largeArrayMechanism}`
)
}
}

Expand Down Expand Up @@ -753,7 +781,7 @@ function buildArrayTypeCondition (type, accessor) {
break
default:
if (Array.isArray(type)) {
const conditions = type.map((subType) => {
const conditions = type.map(subType => {
return buildArrayTypeCondition(subType, accessor)
})
condition = `(${conditions.join(' || ')})`
Expand Down Expand Up @@ -801,10 +829,20 @@ function buildValue (location, input) {
let code = ''
let funcName

if (schema.fjs_type === 'string' && schema.format === undefined && Array.isArray(schema.type) && schema.type.length === 2) {
if (
schema.fjs_type === 'string' &&
schema.format === undefined &&
Array.isArray(schema.type) &&
schema.type.length === 2
) {
type = 'string'
}

if ('const' in schema) {
code += `json += '${JSON.stringify(schema.const)}'`
return code
}

switch (type) {
case 'null':
code += 'json += serializer.asNull()'
Expand Down Expand Up @@ -865,13 +903,6 @@ function buildValue (location, input) {
code += `
json += JSON.stringify(${input})
`
} else if ('const' in schema) {
code += `
if(ajv.validate(${JSON.stringify(schema)}, ${input}))
json += '${JSON.stringify(schema.const)}'
else
throw new Error(\`Item $\{JSON.stringify(${input})} does not match schema definition.\`)
`
} else if (schema.type === undefined) {
code += `
json += JSON.stringify(${input})
Expand Down Expand Up @@ -987,7 +1018,10 @@ function extendDateTimeType (schema) {
function isEmpty (schema) {
// eslint-disable-next-line
for (var key in schema) {
if (Object.prototype.hasOwnProperty.call(schema, key) && schema[key] !== undefined) {
if (
Object.prototype.hasOwnProperty.call(schema, key) &&
schema[key] !== undefined
) {
return false
}
}
Expand All @@ -1001,6 +1035,8 @@ module.exports.validLargeArrayMechanisms = validLargeArrayMechanisms
module.exports.restore = function ({ code, ajv }) {
const serializer = new Serializer()
// eslint-disable-next-line
return (Function.apply(null, ['ajv', 'serializer', code])
.apply(null, [ajv, serializer]))
return Function.apply(null, ["ajv", "serializer", code]).apply(null, [
ajv,
serializer
])
}
Loading